--- /dev/null
+.. _coding-guidelines:
+
+Coding Guidelines
+=================
+
+*Note: This document is a work in progress.*
+
+Git commit message style
+------------------------
+
+For Git commit messages we recommend following the `OpenStack commit message
+recommendations <https://wiki.openstack.org/wiki/GitCommitMessages>`__.
+
+General Code headers
+--------------------
+
+License and Copyright headers need to exist at the top of all code files.
+Examples of copyright headers for each language can be seen below.
+
+Note: In case you need multiple Copyright headers simply duplicate the
+Copyright line for additional copyrights
+
+C/C++/Java
+~~~~~~~~~~
+
+::
+
+ /*
+ * Copyright (c) 2016 <Company or Individual>. All rights reserved.
+ *
+ * This program and the accompanying materials are made available under the
+ * terms of the Eclipse Public License v1.0 which accompanies this distribution,
+ * and is available at http://www.eclipse.org/legal/epl-v10.html
+ */
+
+Bash/Python
+~~~~~~~~~~~
+
+::
+
+ ##############################################################################
+ # Copyright (c) 2016 <Company or Individual>. All rights reserved.
+ #
+ # This program and the accompanying materials are made available under the
+ # terms of the Eclipse Public License v1.0 which accompanies this distribution,
+ # and is available at http://www.eclipse.org/legal/epl-v10.html
+ ##############################################################################
+
+XML
+~~~
+
+::
+
+ <!--
+ Copyright (c) 2016 <Company or Individual>. All rights reserved.
+
+ This program and the accompanying materials are made available under the
+ terms of the Eclipse Public License v1.0 which accompanies this distribution,
+ and is available at http://www.eclipse.org/legal/epl-v10.html
+ -->
+
+General Code Style
+------------------
+
+- 120 characters line length
+
+Java
+----
+
+In General we follow the Google Java Code Style Guide with a few exceptions.
+See: https://google.github.io/styleguide/javaguide.html
+
+- 4 spaces indentation
+- 120 characters line length
+- 72 or 80 characters for javadoc
+- import ordering
+ (https://wiki-archive.opendaylight.org/view/GettingStarted:_Eclipse_Setup#Import_ordering)
+- YangTools Design Guidelines
+ (https://wiki-archive.opendaylight.org/view/YANG_Tools:Design_and_Coding_Guidelines)
+- follow JLS modifier ordering
+ (http://checkstyle.sourceforge.net/config_modifier.html)
+- do not use underscores (_) in identifiers, as they trigger warnings with JDK8
+- Uppercase " *static final Logger LOG = ...* " (See 5.2.4 examples)
+
+Checkstyle
+~~~~~~~~~~
+
+All OpenDaylight projects automatically run Checkstyle, as the
+maven-checkstyle-plugin is declared in the odlparent.
+
+Checkstyle Warnings are considered as errors by default since Magnesium.
+They will prevent your project from building if code violations are found.
+Checkstyle enforcement can be disabled by defining the following property in the
+related pom.xml:
+
+ .. code-block:: none
+
+ <properties>
+ <odlparent.checkstyle.enforce>false</odlparent.checkstyle.enforce>
+ </properties>
+
+Utility classes with only static methods
+^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
+
+In general we recommend that you typically do not overuse utility classes with
+only static methods, but instead write helper @Singleton classes without static
+methods which you can easily @Inject via Dependency Injection into other classes
+requiring them.
+This makes it easier to use non-static helpers in other utilities which can then
+in turn be @Inject into your helper (which you cannot do with static).
+It also makes it easier to mock the helpers for use in unit tests.
+
+If you must write utility classes with only static methods, or have existing
+code that is not trivial to change, then please mark the respective class final,
+and give it a private constructor.
+Please do not throw any exception from the private constructor
+(it is not required).
+
+Suggested process (steps) to move a non-compliant project to enforcement
+^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
+
+We recommend moving existing at least large projects (which typically have
+hundreds or thousands of Checkstyle violations) to full compliance and
+enforcements through a series of Gerrit changes on single artefacts (bundles),
+as opposed to a single change fixing everything and change the POM to enable
+enforcement all in one go (god forbid for an entire repository and not just a
+single artifact), because:
+
+#. single review would be virtually impossible to even remotely sensibly
+ code review by committers
+#. batching style changes by type is easy to review (and approve lines
+ in bulk "by trust"), for example:
+
+ #. *(...project name...) Organize Imports for Checkstyle compliance*
+ #. *(...project name...) Checkstyle compliance: line length*
+ #. *(...project name...) Checkstyle compliance: various types of
+ small changes*
+ #. ''(...project name...) Checkstyle compliant Exception handling'
+ #. ''(...project name...) Checkstyle final clean up & enforcement'
+
+#. it's particularly important to split and separately submit
+ "trivial pure cosmetic" (e.g. code formatting) from "interesting impactful"
+ (e.g. changes to exception handling) changes required by Checkstyle
+#. in general, doing small steps and intermediate merges are more rewarding for
+ contributing developers than long running massive Gerrits
+#. more small changes makes the contributors Stats Great Again
+ (ODL top contributors submit massive amounts of micro changes)
+
+During such a process, it should be considered "normal" and perfectly
+acceptable, that new intermediately merged changes introduce some (small)
+regressions and "re-dirty" some previously cleaned up files;
+it's OK that they'll be re-fixed as part of new changes by the developers
+contributing the clean up in new changes (or rebases)
+- until enforcement is enabled at the end of a series of clean up changes.
+
+@SuppressWarnings
+^^^^^^^^^^^^^^^^^
+
+If really needed, projects can override individual Checkstyle rules on a
+case-by-case basis by using a @SuppressWarnings annotation:
+
+ .. code-block:: none
+
+ @SuppressWarnings("checkstyle:methodparampad")
+ public AbstractDataTreeListener (INetvirtSfcOF13Provider provider, Class<T> clazz) {
+ }
+
+The rule ID (e.g. 'checkstyle:methodparampad' above) is the name of the
+respective Checkstyle module; these IDs can be found e.g. in the
+git/odlparent/checkstyle/src/main/resources/odl_checks.xml
+configuration, or directly on the Checkstyle website from the
+http://checkstyle.sourceforge.net/checks.html list.
+For example, for the
+http://checkstyle.sourceforge.net/config_coding.html#EqualsHashCode rule
+you would put @SuppressWarnings("checkstyle:EqualsHashCode").
+
+This @SuppressWarnings("checkstyle:...") should in practice be very very rarely
+needed.
+Please put a comment explaining why you need to suppress a Checkstyle warning
+into the code for other to understand, for example:
+
+ .. code-block:: none
+
+ @Override
+ @SuppressWarnings("checkstyle:EqualsHashCode"
+ // In this particular case an equals without hashCode is OK because
+ // [explain!] (I'm a certified grown up and know what I'm doing.)
+ public boolean equals(Object obj) {
+
+Please contact odlparent-dev@lists.opendaylight.org if you feel a Checkstyle
+rule is too strict in general and should be reviewed.
+
+The `Evolving Checkstyle old wiki page <https://wiki-archive.opendaylight.org/view/EvolvingCheckstyle>`__
+documents how to test changes to Checkstyle rules.
+
+Notes for particular Checks
+^^^^^^^^^^^^^^^^^^^^^^^^^^^
+
+{@inheritDoc} JavaDoc
+'''''''''''''''''''''
+
+This JavaDoc is useless visual noise that hinders code readability.
+It is not required to put this, and has no impact. JavaDoc does this by default:
+
+.. code:: java
+
+ /**
+ * {@inheritDoc}
+ */
+ @Override // (or on a constructor)
+
+The only case where {@inheritDoc} is useful is when you actually have
+additional Java documentation.
+Default JavaDoc interprets as replace the parent documentation.
+If you truly want the full text of the parent to be copy/pasted by JavaDoc in
+addition to your additional one, then use:
+
+.. code:: java
+
+ /**
+ * {@inheritDoc}
+ * For this specific implementation...
+ */
+ @Override // (or on a constructor)
+
+See also
+https://github.com/sevntu-checkstyle/sevntu.checkstyle/issues/467 &
+http://tornorbye.blogspot.ch/2005/02/inheriting-javadoc-comments.html
+
+IllegalThrows
+'''''''''''''
+
+Instead of declaring "throws Exception", in almost all cases you should instead
+throw a custom existing or new ODL Exception.
+Instead of an unchecked exception (unchecked = extends RuntimeException;
+if you must for some technical reason, but should be rare, and avoided),
+it's recommended to use a custom module specific checked exception
+(checked = extends Exception);
+which can wrap a caught RuntimeException, if needed.
+
+In order to avoid proliferation of many kinds of checked Exception subtypes for
+various particular nitty gritty things which could possibly go wrong, note that
+it in ODL is perfectly OK & recommended to have just ONE checked exception for a
+small given functional ODL module (subsystem), and throw that from all of its
+API methods.
+This makes sense because a typical caller wouldn't want do anything different -
+what you are "bubbling up" is just that one of the operations which one module
+asked another ODL module to do couldn't be performed.
+This avoids having multiple throws for each exception in API methods, and having
+problems with extendibility due to having to add more exceptions to the "throws"
+declaration of API methods.
+
+The exception for "throws Exception" may be a main() method where it's customary
+to let anything propagate to the CLI, or @Test testSomething() throws Exception
+where it's acceptable (Checkstyle does NOT flag this particular use of
+"throws Exception" in @Test methods).
+
+IllegalCatch
+''''''''''''
+
+The `IllegalCatch <http://checkstyle.sourceforge.net/config_coding.html#IllegalCatch>`__
+violation should almost never be suppressed in regular "functional" code.
+Normal code should only catch specific sub classes of the checked Exception,
+and never any generic and/or unchecked exceptions.
+
+In the old pre-Java 7 days, some people used "catch (Exception e)" to
+"save typing" as a shorthand for having to catch a number of possibly thrown
+types of checked exceptions declared with "throws" by a method within the try
+block.
+Nowadays, `since Java 7, using a multi-catch block <http://docs.oracle.com/javase/7/docs/technotes/guides/language/catch-multiple.html>`__
+is the right way to do this.
+In addition to being "nicer" to read because it's clearer, much more importantly
+than, a multi-catch does not also accidentally catch RuntimeException, as catch
+(Exception e) would.
+Catching RuntimeException such as NullPointerException & Co. is typically not
+what the developer who used "catch (Exception e)" as shorthand intended.
+
+If a catch (Exception e) is used after a try { } block which does not call any
+methods declaring that they may throw checked exceptions with their throws
+clause (perhaps not anymore, after code was changed), then that catch may really
+have been intended to catch any possible RuntimeException instead?
+In that case, if there exceptionally really is a particular reason to want to
+"do something and recover from anything that could possibly go wrong, incl.
+NullPointerException, IndexOutOfBoundsException, IllegalArgumentException,
+IllegalStateException & Co.", then it is clearer to just catch
+(RuntimeException e) instead of catch (Exception e).
+Before doing this, double check that this truly is the intention of that code,
+by having a closer look at code called within the try,
+and see if that called code couldn't simply be made more robust.
+Be particularly careful with methods declaring checked exceptions in their
+“throws” clause:
+if any matching exception is thrown inside the “try” block, changing
+“catch (Exception e)” to “catch (RuntimeException e)” could change the method
+behaviour since the exception will exit the method instead of being processed by
+the “catch” block.
+
+Proliferation of catch (Exception or RuntimeException e)
+{ LOG.error("It failed", e); } in regular "functional" code is a symptom of a
+missing abstraction in framework code; e.g. an Abstract interface implementation
+helper with correct default error handling, so that functional code does
+not have to repeat this over and over again.
+For example:
+
+#. For DataBroker related transaction management, check out the (at the time of
+ writing still in review) new utilities from
+ `c/63372 <https://git.opendaylight.org/gerrit/#/c/63372/>`__ & Co.
+#. For RPC related catch, see
+ `c/63413 <https://git.opendaylight.org/gerrit/#/c/63413/>`__
+#. Instead of catch(Exception) after a try { close(anAutoCloseable) }
+ just use AutoCloseables.closeOrWarn(anAutoCloseable) introduced in
+ https://git.opendaylight.org/gerrit/#/c/44145/
+
+Sometimes developers also simply don't see that an existing framework API
+intends implementations to simply propagate their errors up to them.
+For example, for Exception handling in:
+
+#. OsgiCommandSupport's doExecute(), the right thing to do is... nothing.
+ The parent doExecute() method declaration throws Exception,
+ and that is intentional by the Good People of Karaf.
+ Thefore, catch(Exception) in a OsgiCommandSupport's doExecute is not required
+ : in this case it's perfectly OK to just let any error "propagate" upwards
+ automatically.
+ If doExecute() calls other private methods of an OsgiCommandSupport
+ implementation, then it is perfectly OK to make those methods declare
+ "throws SomeException" too, and not "handle" them yourself.
+
+#. Callable's call() passed to a DataStoreJobCoordinator enqueueJob(),
+ the right thing to do is... nothing.
+ Do not catch (Exception) but let it propagate.
+ If it's useful to "augment" the exception message with more custom details
+ which are available inside Callable's call(), then the right thing to do is
+ to catch (Exception e)
+ { throw new YourProjectApiException("Failed to ... for {}", aDetail, e); }
+ and, exceptionally, use @SuppressWarnings("checkstyle:IllegalCatch").
+
+#. org.opendaylight.infrautils.inject.AbstractLifecycle's start() and stop()
+ methods, again the right thing to do is... nothing.
+ Do not catch any Exception but let it propagate.
+
+Here are some cases where catch(Exception) is almost always wrong, and a
+respective @SuppressWarnings almost never acceptable:
+
+#. In Tests code you typically just "@Test testSomething() throws
+ (Some)Exception" instead of catch,
+ or uses @Test(expected = ReadFailedException.class).
+ One rare case we have seen where it's justified is a
+ @Test(expected = ReadFailedException.class)
+ with catch (Exception e) throw e.getCause().
+
+#. In one time "setup" (initialization) kind of code.
+ For example, catch for a DataBroker registerDataChangeListener makes little
+ sense - it's typically much better to let a failure to register a data change
+ listener "bubble up" then continue, even if logged, and have users wonder why
+ the listener isn't working much later.
+
+Only in lower-level "Framework" kind of code, catch (Exception e) is sometimes
+justified / required,
+and thus @SuppressWarnings("checkstyle:IllegalCatch") acceptable.
+
+Catching Throwable in particular is considered an absolute No No
+(see e.g. discussion in https://git.opendaylight.org/gerrit/#/c/60855/)
+in almost all cases.
+You may got confused any meant to catch Exception (see above)
+or RuntimeException?
+
+Carefully consider whether you mean to catch and set some flag and/or
+log, and then rethrow, or intended to "swallow" the exception.
+
+System.out
+''''''''''
+
+The RegexpSingleLineJava "Line contains console output" and "Line contains
+printStackTrace" should NEVER be suppressed.
+
+In src/main code, System.out.println has no place, ever (it should probably be
+a LOG.info; and System.err probably a LOG.error).
+
+In Java code producing Karaf console output, we should only use System.out, and
+add the corresponding @SuppressWarnings. System.out handles pipes and remote
+sessions correctly.
+
+In src/test code, there should be no need to write things out - the point of a
+test is to assert something.
+During development of a test it's sometimes handy to print things to the console
+to see what's going on instead of using the debugger, but this should be removed
+in final code, for clarity, and non-verbose test execution.
+If you must, do you a Logger even in a test - just like in src/main code.
+This also makes it easier to later move code such as helper methods from test to
+production code.
+
+Javadoc Paragraph: < p > tag should be preceded with an empty line
+''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''
+
+Checkstyle (rightfully) flags this kind of JavaDoc up as not ideal for
+readability:
+
+.. code:: java
+
+ /**
+ * Utilities for...
+ * <p>This...
+
+and you can address this either like this:
+
+.. code:: java
+
+ /**
+ * Utilities for...
+ *
+ * <p>This...
+
+or like this:
+
+.. code:: java
+
+ /**
+ * Utilities for...
+ * <p/>
+ * This...
+
+Different ODL developers `agree to disagree <https://git.opendaylight.org/gerrit/#/c/46842/>`__
+on which of the above is more readable.
+
+Additional Resources
+^^^^^^^^^^^^^^^^^^^^
+
+- Checkstyle http://checkstyle.sourceforge.net/
+- Maven:
+ https://github.com/checkstyle/checkstyle/blob/master/src/main/resources/google_checks.xml
+- Eclipse:
+ https://code.google.com/p/google-styleguide/source/browse/trunk/eclipse-java-google-style.xml
+- IntelliJ:
+ https://code.google.com/p/google-styleguide/source/browse/trunk/intellij-java-google-style.xml
+- `How to set Checkstyle up in IntelliJ old wiki page <https://wiki-archive.opendaylight.org/view/How_to_set_Checkstyle_up_in_IntelliJ>`__
+
+PMD Copy/Paste Detection (CPD)
+~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+
+odlparent includes `PMD <https://pmd.github.io>`__ with its `CPD <https://pmd.github.io/pmd-6.0.0/pmd_userdocs_cpd.html>`__
+(Copy/Paste detection), which will warn but yet not fail the build for any
+duplicate code (not just within but also across classes within the same module)
+.
+You should refactor any such copy/pasted code, and can then enforce CPD to fail
+the build for future non regression by adding this to your POM:
+
+.. code-block:: text
+
+ <pmd.cpd.fail>true</pmd.cpd.fail>
+
+Note that CPD's analysis is text-based and not semantic, so it will flag code
+which "looks" identical to it even if it uses different Java types (which do not
+share a common super type; so that it's non-trivial to refactor).
+So in the unlikely case that there is a real good justified reason for what
+looks like copy paste to PMD, you can selectively suppress true PMD CPD false
+positives for a few lines, a method or god forbid an entire class, using:
+
+.. code-block:: text
+
+ @SuppressWarnings("CPD-START")
+ ...
+ @SuppressWarnings("CPD-END")
+
+Classes methods / fields ordering
+~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+
+Ordering based on modifiers. This is based on visibility and mutability:
+
+| public static final fields
+| static final fields
+| private static final fields
+| private final fields
+| private non-final fields
+| private volatile fields
+| private constructors
+| public constructors
+| static factory methods
+| public methods
+| static methods
+| private methods
+| The first group should be very strict, with the exception of
+ FieldUpdaters, which should be private static final, but defined just
+ above the volatile field they access. The reason for that is they are
+ tied via a string literal name.
+
+The second group is less clear-cut and depends on how instances are created --
+there are times when juggling the order makes it easier to understand what is
+going on (e.g. co-locating a private static method with static factory method
+which uses it).
+
+The third group is pretty much free-for-all.
+The goal is to group things so that people do not have scroll around to
+understand the code flow.
+Public methods are obviously entry-points,
+hence are mostly glanced over by users.
+
+Overall this has worked really well so far because;
+
+- the first group gives a 10000-foot overview of what is going on in the class,
+ its footprint and references to other classes
+- the second group gives instantiation entry-points, useful for examining who
+ creates the objects and how
+- the third group is implementation details -- for when you really need to dive
+ into the details.
+
+Note this list does not include non-private fields.
+The reason is that public fields should be forbidden, as should be any mutable
+non-private fields.
+The reason for that is they are a nightmare to navigate when attempting to
+reason about object lifecycle.
+
+Same reasoning applies to inner class, keep them close to the methods which use
+them so that the class is easy to read.
+If the inner class needs to be understood before the methods that operate on it,
+place it before them, otherwise (especially if it's an implementation detail)
+after them.
+That's when an inner class is appropriate of course.
+
+error-prone
+~~~~~~~~~~~
+
+The infrautils projects has adopted the `error-prone code quality tool <http://errorprone.info>`__
+(by Google), with suitable customized configuration.
+
+You can use it by using org.opendaylight.infrautils:parent instead of
+org.opendaylight.odlparent:bundle-parent.
+
+Note that @SuppressWarnings("InconsistentOverloads") needs to be placed on the
+class, not method; see
+https://github.com/google/error-prone/pull/870 and
+https://github.com/google/error-prone/issues/869.
+
+SpotBugs
+~~~~~~~~
+
+SpotBugs is the sucesssor project to FindBugs (which is dead).
+
+SpotBugs is enforced by default across all artifacts since Magnesium.
+For artifacts that do not pass SpotBugs, either fix them or disable enforcement
+by defining the following property in the pom.xml:
+
+ .. code-block:: none
+
+ <properties>
+ <odlparent.spotbugs.enforce>false</odlparent.spotbugs.enforce>
+ </properties>
+
+All notes re. FindBugs listed below do still apply to SpotBugs as well
+(it's compatible).
+
+FindBugs
+~~~~~~~~
+
+Note that starting with odlparent 3.0.0 when we say "FindBugs" we now actually
+mean "SpotBugs", see above.
+
+@FBSuppressWarnings
+^^^^^^^^^^^^^^^^^^^
+
+If really needed, projects can to override individual FindBugs rules on
+a case-by-case basis by using a @SuppressFBWarnings annotation:
+
+.. code:: java
+
+ @SuppressFBWarnings("RCN_REDUNDANT_NULLCHECK_OF_NONNULL_VALUE")
+ public V get(K key) {
+
+Unchecked/unconfirmed cast from com.google.common.truth.Subject to com.google.common.truth.BooleanSubject etc.
+^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
+
+FindBugs seems to be too dumb to deal with perfectly valid Google Truth test
+code (which does not use any explicit cast...) so it's OK to:
+
+.. code:: java
+
+ @SuppressFBWarnings("BC_UNCONFIRMED_CAST")
+
+an entire JUnit \*Test class because of that.
+
+null analysis errors, incl. FindBugs' NP_NONNULL_FIELD_NOT_INITIALIZED_IN_CONSTRUCTOR
+^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
+
+see the general null analysis next chapter.
+
+nonNullAndOptional
+~~~~~~~~~~~~~~~~~~
+
+Some of the content from this chapter may be moved to
+http://www.lastnpe.org later...
+
+Everything @NonNullByDefault
+^^^^^^^^^^^^^^^^^^^^^^^^^^^^
+
+Do not use null anywhere, assume all method arguments and return values are
+NonNullByDefault.
+
+null annotations from org.eclipse.jdt.annotation instead of javax.annotation
+^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
+
+We prefer using the null annotations from the package org.eclipse.jdt.annotation
+, instead of the ones from javax.annotation
+(or those from org.jetbrains:annotations, or ... Android/Lombok's/some of the
+other ones out there).
+
+This is because the org.eclipse one are modern generics enabled @Target
+TYPE_USE annotations, whereas the other ones are not.
+
+Obviously we do NOT "depend on Eclipse" in any way, or "need Eclipse at
+run-time" just because of 4 annotations from an org.eclipse package,
+which are available in a very small JAR at build-time; e.g.
+org.eclipse.jdt.annotation.NonNull is absolutely no different from
+javax.annotation.Nullable, in that regard.
+
+BTW: The javax.annotation NonNull & Co. are not more or less "official"
+than others; Prof. FindBugs Bill Pugh pushed those to Maven central, but
+his "dormanant" JSR 305 was never officially finalized and approved;
+he's since moved on, and no longer maintains FindBugs.
+
+The Eclipse annotations are also supported by SpotBugs/FindBugs (`says
+this issue <https://github.com/spotbugs/spotbugs/issues/471>`__) as well
+as NullAway.
+
+null analysis by FindBugs VS. Eclipse JDT
+^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
+
+FindBugs' null analysis is inferior to Eclipse JDT's, because:
+
+- richer null analysis
+- generics enabled (see above)
+- works with existing external libraries, through external annotations,
+ see http://www.lastnpe.org
+- much better in-IDE support, at least for Eclipse users
+
+*WIP: We are aiming at, eventualy, running headless Eclipse JDT-based null
+analysis during the build, not just for users of the Eclipse IDE UI;
+watch*\ `issue ODLPARENT-116 <https://jira.opendaylight.org/browse/ODLPARENT-116>`__
+\ *, and*\ http://www.lastnpe.org\ *.*
+
+BTW: FindBugs is dead anyway, long live SpotBugs! \_TODO Gerrit & more
+documentation to clarify this...\_
+
+PS: An alternative interesting null checker tool is the `Checker
+Framework <https://checkerframework.org>`__.
+
+Runtime null checks
+^^^^^^^^^^^^^^^^^^^
+
+In addition to static null analysis during development, you can check null
+safety at run-time.
+Please use either `JDK's Object's requireNonNull <http://docs.oracle.com/javase/7/docs/api/java/util/Objects.html#requireNonNull(java.lang.Object,java.lang.String)>`__
+\ () or `Guava's
+Preconditions <https://github.com/google/guava/wiki/PreconditionsExplained>`__
+checkNotNull() utility, instead of if (something == null).
+Please also use the variant of requireNonNull() or checkNotNull() with the
+String message to indicate what argument is checked.
+For example:
+
+.. code:: java
+
+ public doSomething(Something something) {
+ this.something = Objects.requireNonNull(something, "something");
+ }
+
+We recommend use of JDK's Object's requireNonNull() instead of Guava's
+Preconditions checkNotNull() just because the String message of the former can
+prevent the problem you can have with the latter if you confuse the order of the
+arguments.
+
+We accept and think its OK that checkNotNull() throws a NullPointerException and
+not an IllegalArgumentException (even though code otherwise should never
+manually throw new NullPointerException),
+because in this particular case a NullPointerException would have happened
+anyway later, so it's just an earlier NullPointerException, with added
+information of what is null.
+
+We NEVER catch (NullPointerException e) anywhere, because they are programming
+errors which should "bubble up", to be fixed, not suppressed.
+
+nullable errors for fields related to dependency injection
+^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
+
+null analysis frameworks, such as Eclipse's or FindBugs or others, will not like
+this kind of code in a @NonNullByDefault package:
+
+.. code:: java
+
+ class Example {
+ @Inject
+ Service s;
+ }
+
+the recommended solution is to always use constructor instead of field
+injection instead, like this:
+
+.. code:: java
+
+ class Example {
+ final Service s;
+ @Inject
+ Example(Service s) {
+ this.s = s;
+ }
+ }
+
+When this exceptionally is not possible, like in a JUnit component test,
+then it's acceptable to suppress warnings:
+
+.. code:: java
+
+ @SuppressFBWarnings("NP_NONNULL_FIELD_NOT_INITIALIZED_IN_CONSTRUCTOR")
+ class SomeTest {
+ public @Rule GuiceRule guice = new GuiceRule(TestModule.class);
+ private @Inject Service service;
+ }
+
+Optional
+^^^^^^^^
+
+You do not have to use Optional, because real null analysis can give us the same
+benefit.
+
+If cleaning up code for null safety, then do not mix introducing Optional with
+other null related clean up changes; it's clearer for reviews if you FIRST fix
+missing null checks and add null related annotations, and then THEN (optionally)
+switch some return types to Optional.
+
+You can use Optional for return types, but not method arguments.
+
+Never use Optional<Collection<...>> (obviously incl. Optional<List<...>>
+or Optional<Set<...>> AND Optional<Map<...>> etc.),
+just return a respective empty Collection instead.
+
+Note that instead of if (anOptional.isPresent()) { return anOptional.get(); }
+else { return null; }
+you can and for readability should just use return anOptional.orNull().
+However ideally any such code should not return null but an Optional of
+something itself.
+
+Note that instead of if (aNullable == null) ? Optional.absent() :
+Optional.of(aNullable)a
+;you can and for readability should just use Optional.fromNullable(aNullable).
+
+To transform an Optional into something else if it present, use the transform()
+method instead of an if () check;.
+for example:
+
+.. code:: java
+
+ List vrfEntries = MDSALUtil.read(broker, LogicalDatastoreType.CONFIGURATION, vpnVrfTables).transform(VrfTables::getVrfEntry).or(new ArrayList<>());
+
+**Take care** with Optional.transform() though: if the transformation can return
+null, Optional.transform() will fail with a NullPointerException
+(this is the case of most YANG-generated methods).
+You can use Java 8 Optional.map() instead; when it encounters null, it returns
+Optional.empty().
+The above example would become
+
+.. code:: java
+
+ List<VrfEntry> vrfEntries = MDSALUtil.read(broker, LogicalDatastoreType.CONFIGURATION, vpnVrfTables).toJavaUtil().map(VrfTables::getVrfEntry).orElse(new ArrayList<>());
+
+Prefer the newer Java 8 java.util.Optional (JUO) over the older Google Guava
+com.google.common.base.Optional (GGO), because it offers a better functional
+style API for use with Java 8 lambdas, which leads to much more naturally
+readable code.
+Until we fully migrate all ODL APIs (which is planned), in glue code calling
+existing APIs returning GGO (such as the DataBroker API) but itself then wanting
+to return a function of that as JUO, please just use the toJavaUtil() method
+available in Guava Optional.
+
+Further Reading & Watching
+''''''''''''''''''''''''''
+
+- https://github.com/google/guava/wiki/UsingAndAvoidingNullExplained
+- https://stackoverflow.com/questions/26327957/should-java-8-getters-return-optional-type
+
+Streaming and lambdas
+~~~~~~~~~~~~~~~~~~~~~
+
+Lambdas are very useful when encapsulating varying behaviour.
+For example, you can use them instead of boolean behaviour selection parameters:
+
+.. code:: java
+
+ public void someMethodA(SomeObject A) {
+ commonMethod(A, false);
+ }
+
+ public void someMethodB(SomeObject B) {
+ commonMethod(B, true);
+ }
+
+ private void commonMethod(SomeObject C, boolean behaviour) {
+ // common code
+
+ if (behaviour) {
+ doA();
+ } else {
+ doB();
+ }
+
+ // common code
+ }
+
+can be replaced with
+
+.. code:: java
+
+ public void someMethodA(SomeObject A) {
+ commonMethod(A, this::doA);
+ }
+
+ public void someMethodB(SomeObjectB) {
+ commonMethod(B, this::doB);
+ }
+
+ private void commonMethod(SomeObject C, Function behaviour) {
+ // common code
+
+ behaviour.apply();
+
+ // common code
+ }
+
+They are also useful for replacing small anonymous inner classes which follow
+the functional pattern (implementing an interface with a single non-default
+method).
+Your IDE will typically suggest such transformations.
+
+Lambdas should be avoided when the resulting code is more complex and/ora longer
+than the non-functional form.
+This can happen particularly with streaming.
+
+Streaming also has its place, especially when combined with Optional (see above)
+or when processing collections.
+It should however be avoided when many objects are created in the resulting
+lamba expressions, especially if DTOs end up being necessary to convey
+information from one lambda to the next where a single variable could do the
+trick in a more traditional form. (TODO: provide examples.)
+
+Python
+------
+
+PEP8 is the Python standard that should be followed when coding any Python code
+with the following exceptions.
+
+- 120 characters line length
+
+To automate pep8 scanning we recommend using a tox configuration as follows:
+
+tox.ini
+
+::
+
+ [tox]
+ envlist = pep8
+ #skipsdist = true # Command only available in tox 1.6.0
+
+ [testenv:pep8]
+ deps = flake8
+ commands = flake8
+
+ [flake8]
+ max-line-length = 120
+
+Unfortunately the version of tox installed in the Jenkins build infra does not
+support the skipdist parameter which is necessary if your project does not have
+a setup.py file.
+A workaround is to create a minimal setup.py file as follows:
+
+setup.py
+
+::
+
+ # Workaround for tox missing parameter 'skipsdist=true' which was not
+ # introduced until tox 1.6.0
+
+ import setuptools
+
+ setuptools.setup(name='project-name')
+
+.. _xml-1:
+
+XML
+---
+
+- use self-closing
+- include proper namespace/model/version declarations
+- TBD
+
+YANG
+----
+
+- Do not use underscores ('_') in identifiers.
+ JDK 9 is on track to making underscores forbidden in identifiers, which means
+ we will need to map them and it is not going to be pleasant :-(
+- Each declaration needs to have either a description or a reference to a
+ defintion document (like an IETF draft)
+- Use typedefs to declare concepts. An UUID is typeless, so each instance
+ should have its scope, so we know its applicability domain.
+ 'type string' should only be used to things like free-form comments and
+ similar. Please attach a 'units' statement whenever possible.
+- TBD
--- /dev/null
+What is Gerrit ?
+================
+
+The Linux Foundation proposes a LF Gerrit Guide at this URL:
+https://docs.releng.linuxfoundation.org/en/latest/gerrit.html Please
+look at it.
+
+You can find more details on Gerrit by itself at
+https://gerrit-review.googlesource.com/Documentation/intro-quick.html
+
+Gerrit is a web-based code review tool built on top of the Git version
+control system. Gerrit makes code review easier by providing a
+lightweight framework for reviewing commits before they are accepted by
+the codebase i.e. project committers.
+
+If you are not familiar with Git, please look at the `LFN Git guide <https://docs.releng.linuxfoundation.org/en/latest/git.html>`__.
+
+Gerrit uses massively git hooks. Git hooks are commands and scripts to
+add automatically specific treatments in git basic commands. **The main
+hook treatment provided by Gerrit is to add a “Change-Id” tag to all
+commit messages. This “Change-Id” allows Gerrit to link together
+different versions of the same change being reviewed.**
+
+If you want to look at the hooks contents, you can retrieve them
+manually on the Gerrit git server with those commands.
+
+.. code-block:: text
+
+ $ scp -p -P 29418 <login>@git.opendaylight.org:hooks/commit-msg RecipeBook/.git/hooks/
+ $ chmod u+x .git/hooks/commit-msg
+
+Most of the time, retrieving hooks manually is useless since they are
+automatically invoked by git commands. With Gerrit, it is also much
+easier to use git-review that invokes those hooks. **Git-review is a
+tool that helps submitting git branches to Gerrit for review, this means
+it can supersede the usual git fetch, git pull and git push commands.**
+The git-review package is available on most GNU/Linux distribution.
+
+**In Gerrit, several ongoing-review branches or “changes” can coexist
+without conflicts until they are merged to a mainstream branch (usually
+the master).** This way, several paths for the project can be explored
+simultaneously even if they implement a same feature. However, if a
+change is getting too old, it may become impossible to merge because the
+mainstream branch has evolved too much meanwhile. In that case, you have
+to rebase the change on the mainstream branch and resolve potential
+conflicts before it can be merged.
+
+
+Guidelines and practical advice
+===============================
+
+
+clone a project repository and get a local copy of the code
+-----------------------------------------------------------
+
+*To access OpenDaylight repository, you need a Linux Foundation ID to log in.*
+This can be done at `this URL <https://identity.linuxfoundation.org/>`__.
+
+*Once that done, you need to generate your SSH keys pair and publish
+your public on your ODL Gerrit account as described in* `this link <https://git.opendaylight.org/gerrit/Documentation/user-upload.html#ssh>`__.
+
+To clone the current master branch of a project
+
+.. code-block:: text
+
+ $ git clone ssh://<login>@git.opendaylight.org:29418/<project_name>
+
+To clone another mainstream branch
+
+.. code-block:: text
+
+ $ git clone -b <branch_name> ssh://< login >@git.opendaylight.org:29418/<project_name>
+
+for example the magnesium branch of the documentation project
+
+.. code-block:: text
+
+ $ git clone -b stable/magnesium ssh://my_login@git.opendaylight.org:29418/docs
+
+at that step, it is usually a good idea to run the setup commands of the repo
+with git-review
+
+.. code-block:: text
+
+ $ cd docs/
+ $ git-review -s
+
+If needed, a particular change (i.e. a specific branch used for
+staging reviews before they are merged to the master branch or another
+mainstream branch) can be retrieved by using git-review
+
+.. code-block:: text
+
+
+ $ git-review -d <change_number>
+
+This will create another local repository along the master branch copy. You
+can use git branch to verify it.
+
+prepare a change
+----------------
+
+Once you have a local copy of the repository, you can make your modifications.
+Please follow the best practices given below and in :ref:`coding-guidelines`.
+Remember to check what you have done.
+
+**Be particularly careful to the license headers, the trailing blanks, the empty
+lines and do not use the MSDOS file format but the UNIX file format.
+Try also to remove compilation warnings.**
+
+If you are using an IDE , it can be a good idea to use an editor profile that
+implements those rule such as the Eclipse profile in this `link <https://wiki-archive.opendaylight.org/images/c/ca/Profile-Java-ODL.xml.zip>`__.
+
+Since ODL compilation process is particularly verbose, consider using
+compilation logs file or piped commands such as:
+
+.. code-block:: text
+
+ $ time mvn clean install -DskipTests 2>&1 |tee mvn.log.0 |grep ‘WARN\|ERROR’
+
+If you propose an update for an existing feature with tests already available,
+it is a good idea to perform these tests ( e.g. “$ tox -e pre-commit”) and see
+what happens. Available tests can often be found in the tox.ini file at the root
+of the project folder.
+
+Once ready, use git status to check the staging files.
+
+.. code-block:: text
+
+ $ git status
+
+If you want to commit all your changes, you can skip the next 2 steps and
+use directly “$ git commit” with the option “-a”
+
+If not, add the right files to your commit.
+
+.. code-block:: text
+
+ $ git add [ … ]
+
+.. note::
+ You might want to remove some files from the remote repository in this
+ commit.
+ In that case, you should use “git rm” instead of only “rm”.
+
+ The same way, use “git mv” if you want to rename or move a file in the
+ remote repository too.
+
+It is a good idea to check again your git status before committing.
+
+.. code-block:: text
+
+ $ git status
+
+
+Then commit and add a commit message. Using “-s” to sign-off your commit
+is usually a good idea.
+
+.. code-block:: text
+
+ $ git commit -s
+
+.. note::
+ Please abide by the commit messages rules given below and at `the openStack wiki <https://wiki.openstack.org/wiki/GitCommitMessages>`__.
+
+ Be careful with the title length (50 char), the empty line after the title,
+ and the body length (72 char).
+
+ If your commit includes work from other contributors, do not hesitate to use
+ the “co-authored-by” tag.
+
+ If you are not the author of the changes, you can upload it although but you
+ should use the option “--author=” with “git-commit”
+
+ Contributors must agree with `the OpenDaylight Technical charter
+ <https://www.opendaylight.org/wp-content/uploads/sites/14/2018/01/ODL-Technical-Charter-LF-Projects-LLC-FINAL.pdf>`__.
+ The sign-off field is related to the role of the Certificate of Origin
+ explained in this charter.
+
+At that step, you can still rework your modifications and include more
+files with “git add”.
+In that case, amend the commit after with the command.
+
+.. code-block:: text
+
+ $ git commit --amend
+
+This command also allows you to rework your commit message too.
+
+
+Upload a Change
+---------------
+
+Uploading a change to Gerrit is done by pushing a git commit to the
+Gerrit origin server. The commit must be pushed to a ref in the
+refs/for/ namespace which defines the target branch: refs/for/<
+target-branch >. The magic refs/for/ prefix allows Gerrit to
+differentiate commits that are pushed for review from commits that are
+pushed directly into the repository, bypassing code review (this is
+usually a bad idea). For the target branch it is sufficient to specify
+the short name, e.g. master, but you can also specify the fully
+qualified branch name, e.g. refs/heads/master.
+
+Push for Code Review
+
+.. code-block:: text
+
+ $ git commit
+ $ git push origin HEAD:refs/for/master
+
+ // this is the same as:
+ $ git commit
+ $ git push origin HEAD:refs/for/refs/heads/master
+
+It is easier to use the equivalent git-review commands.
+The -T option allows to avoid sending the local branch name as default topic.
+
+.. code-block:: text
+
+ $ git-review -T
+
+If you want to upload it on another mainstream branch for review, you
+can add the branch name at the end.
+
+.. code-block:: text
+
+ $ git-review -T <branch_name>
+
+for example magnesium
+
+.. code-block:: text
+
+ $ git-review -T stable/magnesium
+
+It is also sometimes possible to push commits with bypassing Code Review.
+Beware this is usually a bad idea !
+
+.. code-block:: text
+
+ $ git commit
+ $ git push origin HEAD:master
+
+ // this is the same as:
+ $ git commit
+ $ git push origin HEAD:refs/heads/master
+
+Check your change on Gerrit
+---------------------------
+
+Each file added, modified, moved, renamed or deleted will be listed in the
+Gerrit page displaying your change.
+If you click on a file name, the differences with the previous version of the
+file will be displayed.
+Main common errors such as trailing blanks usually appears in red.
+Please check every file and list those common errors. Then (or in
+parallel) you can go to the next steps and correct them quickly. This is
+a good idea to do it before asking other people to review your change in
+Gerrit.
+
+
+Upload a new Patch Set
+----------------------
+
+If there is feedback from code review and a change should be improved,
+a new patch set with the reworked code should be uploaded.
+
+This is done by amending the commit of the last patch set.
+
+*If you have no more a local copy of your change, you can use “git
+clone” and “git-review -d” to retrieve it (just as described in the
+first section "*
+The commit can also be fetched from Gerrit by using the fetch commands
+available from the download commands in the change screen (right top corner).
+
+.. code-block:: text
+
+ // fetch and checkout the change
+ // (checkout command can be copied from change screen, right top corner in download)
+ $ git fetch "https://git.opendaylight.org/gerrit/docs" refs/changes/86/93386/2
+ && git checkout FETCH_HEAD
+
+ // or with git-review
+ $ git-review -d 93386
+ // provided 2 is the latest Patch Set, otherwise if there is a Patch Set 3,
+ $ git-review -d 93386,2
+ // specifying a Patch-Set number only works with newer git-review versions
+
+Then you can start working on it just as you will do for a new commit with “git
+add/rm/mv etc…”. Once ready, instead of simply doing “$ git commit -s”
+type instead “$ git commit --amend”
+
+
+.. code-block:: text
+
+ // rework the change
+ $ git add < path-of-reworked-file > [ … ]
+
+ // amend commit
+ $ git commit --amend
+
+ // push patch set
+ $ git push origin HEAD:refs/for/master
+ // or with git-review
+ $ git-review -T
+
+
+It is important that the commit message contains the Change-Id of the
+change that should be updated as a footer (last paragraph). Normally the
+commit message already contains the correct Change-Id and the Change-Id
+is preserved when the commit is amended.
+
+Thanks to the Change-Id in the commit message, Gerrit will detect that
+the change was already there and that you want to create a new Patch Set
+to amend it. The new Patch Set should now appear in the Gerrit web
+interface.
+
+.. note::
+ Never amend a commit that is already part of a central branch.
+
+ Pushing a new patch set will trigger an email notification to the reviewers
+ who subscribed to the project notifications.
+
+ The option -T is used to avoid adding a topic to the change. If no topic
+ is specified, git-review will add the change number or the local branch name
+ as a topic in Gerrit web interface.
+ You can force another topic with the -t option.
+
+
+Submitting simultaneously several changes for review
+----------------------------------------------------
+
+Sometimes, it can be interesting to push simultaneously several
+interdependent changes for review. This can be done this way.
+
+.. code-block:: text
+
+ $ git add […]
+
+ $ git commit -s
+
+ [ … ]
+
+ $ git add […]
+
+ $ git commit -s
+
+ $ git-review
+
+Here is a simple example that modifies an existing change and proposes a new
+change on top of it.
+
+.. code-block:: text
+
+ $ git clone https://git.opendaylight.org/gerrit/docs
+ $ git-review -s
+ $ git-review -d 93386
+
+ // rework the change 93386
+ [..]
+ $ git add < path-of-reworked-file > [ … ]
+
+ // amend commit
+ $ git commit –amend
+
+ // add a new change/commit
+ $ git add < path-of-worked-file > [ … ]
+ // add a new commit
+ $ git commit
+
+ // repeat the operation as much time as necessary
+ [..]
+
+ // upload the changes to Gerrit
+ $ git-review -T
+
+git-review usually displays a warning and ask confirmation when doing this.
+The option -y avoids this message.
+
+If the changes are accepted, the Gerrit web interface will display information
+a.k.a. relation chain on changes submitted together when looking at one of them.
+
+.. note::
+ When cascading more changes, the first call “git-review” may fail
+ because of the absence of a Change-Id in the git commit message logs.
+
+ Retry “git-review” in that case or try to run git hook manually to
+ modify the git log history (not so easy).
+
+ If you do not have Gerrit git hooks pre-installed, this only works for the
+ absence of Change-Id in the last commit.
+ In that case, you can use interactive rebase with reword to edit the N
+ previous commit messages (“git-rebase -i HEAD~N”).
+
+
+Modify several changes
+----------------------
+
+While they have not been merged in the remote repository, it is still possible
+to rework the changes that were posted simultaneously. If you have no more
+a local copy of them, just retrieve the latest change in you git history
+from the Gerrit remote repo. Check the history with
+
+.. code-block:: text
+
+ $ git log
+
+It should display all the commits posted.
+
+*“git commit --amend”* only allows to rework the last commit. You must
+use another method to rework the previous commits.
+
+The easiest way to do that is to use interactive rebase 2 syntaxes can
+be used:
+
+.. code-block:: text
+
+ $ git rebase -i < commit >
+
+where is the commit hash reference used by “git log”
+
+or
+
+.. code-block:: text
+
+ $ git rebase -i HEAD~< number of commits >
+ // e.g. to rework the five previous commits
+ $ git rebase-i HEAD~5
+
+you should now see commits short descriptions in a text editor (usually
+vim) It should look like this.
+
+.. code-block:: text
+
+ pick 239da71 Renderer and OLM update
+ pick f85398e Bugs correction in Portmapping
+ pick 6cb0144 Minor checkstyle corrections
+ pick e51e0b9 Network topology and inventory init
+ pick f245366 Bugs correction in NetworkModelService
+
+ # Rebase afe9fcf..f245366 onto afe9fcf
+ #
+ # Commands:
+ # p, pick = use commit
+ # r, reword = use commit, but edit the commit message
+ # e, edit = use commit, but stop for amending
+ # s, squash = use commit, but meld into previous commit
+ # f, fixup = like "squash", but discard this commit's log message
+ # x, exec = run command (the rest of the line) using shell
+ #
+ # These lines can be re-ordered; they are executed from top to bottom.
+ #
+ # If you remove a line here THAT COMMIT WILL BE LOST.
+ #
+ # However, if you remove everything, the rebase will be aborted.
+ #
+ # Note that empty commits are commented out
+
+The editor allows you to proceed to several actions on the git history:
+
+- remove a commit from the history: just delete its line
+- rework dependencies: swap line orders
+- meld several commits into one: replace “pick” by “squash” or “fixup”
+- rework only a specific commit message: replace “pick” by “reword”
+- rework a specific commit: replace “pick” by “edit” then “git add/rm/mv …”,
+ “git commit --amend”, “git rebase --continue”
+
+Beware you may have to deal with potential conflicts when doing this.
+
+Note that alternate methods exist.
+For example, you can use cherry-picks described in the next section.
+You can also use non-interactive git rebase , i.e without the option “-i”.
+But you must keep a copy of the original “git log” history.
+Most people create a new local branch with a copy via "git checkout -b" to that
+purpose.
+Once the copy made, use
+
+.. code-block:: text
+
+ $ git checkout <commit_hash>
+
+where < commit_hash > is the hash of a previous commit, let’ say N
+commits before the last one. Do your modifications:
+
+.. code-block:: text
+
+ $ git add/rm/mv […]
+ $ git commit --amend
+
+A new commit hash (<newhash>) will be generated. Keep it.
+
+.. code-block:: text
+
+ $ git checkout <commit_hash-1>
+
+where < commit_hash-1 > is the hash of the previous commit, N-1 commits
+before the last one. If you look at “git log”, the history has not
+changed and the old hash is still there. you need to rebase to apply the
+modifications made in the previous commit.
+
+.. code-block:: text
+
+ $ git log
+ $ git rebase <newhash>
+ $ git log
+
+Conflicts may appear but should be solvable. Proceed the same way with
+the N-2 previous commits up to the last commit. Then upload
+
+.. code-block:: text
+
+ $ git-review
+
+Cherry-picks / backports
+------------------------
+
+Cherry-pick consist in importing the content of a specific change (or
+commit) from another (review) branch into your own local branch.
+
+The basic git cherry-pick method is described in the `LFN Git Guide <https://docs.releng.linuxfoundation.org/en/latest/git.html#cherry-pick-a-commit>`__.
+
+The principle remains the same with Gerrit but you have to deal with the
+Gerrit branch review system. You can use the “git cherry-pick” classical
+command. In that case, you’d better to copy/paste it from the right-top
+corner of the change review page. The easiest option is to use
+git-review with the option “-x” instead.
+
+.. code-block:: text
+
+ $ git-review -x < change_number >"
+
+*You can use also “-X” to keep a trace of the cherry-pick operation in
+the git log. The “-N” option prepare the cherry-pick but the commit
+message is not imported.*
+
+Several cherry-picks can be cascaded this way.
+
+Once the change appears in your local branch, you can upload it to the
+Gerrit remote repository as usual with git-review.
+
+Cherry-pick can also be used to backport changes between several
+mainstream branches of the Gerrit remote repository. The procedure is the
+same. Here is an example.
+
+.. code-block:: text
+
+ $ git clone -b stable/aluminium ssh://< login >@git.opendaylight.org:29418/docs
+ $ git-review -x 94257
+ //Change 94257 is on the magnesium branch and not the aluminium branch
+ $ git-review [–P] [stable/aluminium]
+
+
+Resolving conflicts
+-------------------
+
+Conflict resolution in Gerrit is not different from Git.
+You can also refer to the `LFN Git Guide <https://docs.releng.linuxfoundation.org/en/latest/git.html#git-merge-conflicts>`__.
+
+Conflict can occur during Git merges, pushes or rebases.
+
+For example, if two or more members make changes on the same part of a file in a
+remote and a local branch that are being merged, Git will not be able to
+automatically merge them and you will get a merge conflict. When this
+happens, conflicting files will be listed in the resulting messages as
+in the example below.
+
+.. code-block:: text
+
+ $ git merge issue3
+ Auto-merging my_shopping_list.txt CONFLICT (content): Merge conflict in my_shopping_list.txt
+ Automatic merge failed; fix conflicts and then commit the result.
+
+And Git will add some standard conflict-resolution markers to those
+conflicting files. The markers act as an indicator to help us figure out
+sections in the content of the conflicting file that needs to be
+manually resolved.
+
+Example of a conflict occurrence
+
+.. code-block:: text
+
+ My Shopping list
+
+ Apples
+ <<<<<<< HEAD
+ Bread
+ Pancakes
+ =======
+ Banana
+ Soda
+ >>>>>>> issue3
+ Tomatoes
+
+Each conflicting section in the file is delimited by lines alike
+“<<<<<<< HEAD” and “>>>>>>> issue3” .
+When merging remote code into your local branch, everything above " ======== "
+is your local content, and everything below it comes from the remote branch.
+Before going further, we need to resolve the conflicting parts and removes those
+markers as shown in the example below.
+
+.. code-block:: text
+
+ My Shopping list
+
+ Apples
+ Banana
+ Bread
+ Pancakes
+ Soda
+ Tomatoes
+
+Once we are done with resolving the conflict, you can commit the change
+(git commit -m) , or pursue a rebase if you were in a rebasing process.
+
+
+
+OpenDaylight and common Best Practices
+======================================
+
+All details on OpenDaylight best practices are available at
+`this URL on the old wiki <https://wiki-archive.opendaylight.org/view/BestPractices>`__.
+
+Implicit rules
+--------------
+
+**The first rule is that the author or at least the owner(=uploader) of
+the change is responsible for the code posted on the Gerrit server.
+This means that the author or the owner has to be responsive to questions
+in Gerrit comments.**
+This is especially important for adaptations asked by committers.
+Committers are in charge of making the mainstream branch clean and conform to
+the project rules before merging it in the mainstream branch.
+Other reviews from non-committers are also welcome.
+
+It may sound a little awkward but **many developers consider a “-1” review as
+good news as a “+1” review .Both mean someone has looked at their code and
+posted useful comments, potentially reusable elsewhere.**
+
+There can be several interpretations of what to do in some case and Gerrit
+comments can be very useful to clarify points in case of disagreements.
+If possible, the change uploader/owner must be the code author so that the
+review is more interactive and responsive.
+
+**The second rule is to keep the code posted reviewable. The change
+should not bring regression nor new compilation errors and warnings.**
+It is a good idea to look at the Gerrit interface editor once your code
+has been posted for review. Most common errors scuh as trailing blanks are
+colored in red.
+Those errors pollutes the review process, not only because they generate many
+warnings during the compilation process.
+Posting a quick fix for those most common issues in a new Patch Set will ease
+the reviewers and committers work.
+If you are not confident of what you have done, you can test your change in
+Gerrit by using the private before making it public or by using the
+Work-in-Progress mode to clearly state it is an ongoing work.
+
+**Huge amounts of code are also generally difficult to review. Gerrit
+changes dashboard has a size indicator on the right.**
+There is no strict rule about this but if you receive a XL size, you probably
+should consider to split your change into several smaller ones.
+
+Coding Guidelines and common issues
+-----------------------------------
+
+More details at :ref:`coding-guidelines`.
+
+Commit message
+~~~~~~~~~~~~~~
+
+More details at https://wiki.openstack.org/wiki/GitCommitMessages
+
+**The commit message should reflect the feature or improvements posted
+in the change.** The message must give a good idea of what have been
+done. **It must be understood by anybody with a sufficient knowledge on
+the topic, not necessarily someone taking part to the project.**
+External references are welcome to point out to more details, but they
+should not be a substitute to a correct description.
+
+Here is a summary of Git commit message structure as described in
+`the OpenStack wiki <https://wiki.openstack.org/wiki/GitCommitMessages#Summary_of_Git_commit_message_structure>`__.
+
+- Provide a brief description of the change in the first line.
+- Insert a single blank line after the first line.
+- Provide a detailed description of the change in the following lines,
+ breaking paragraphs where needed.
+- The first line should be limited to 50 characters and should not end
+ with a period.
+- Subsequent lines should be wrapped at 72 characters. There are some
+ exceptions to this rule: for example, URL should not be wrapped. Vim
+ (the default $EDITOR on most distros) can wrap automatically lines
+ for you. In most cases you just need to copy the example vimrc file
+ (which can be found somewhere like
+ /usr/share/vim/vim74/vimrc_example.vim) to ~/.vimrc if you don’t have
+ one already. After editing a paragraph, you can re-wrap it by
+ pressing escape, ensuring the cursor is within the paragraph and
+ typing gqip. Put the ‘Change-id’, ‘Closes-Bug #NNNNN’ and ‘blueprint
+ NNNNNNNNNNN’ lines at the very end.
+
+
+.. note::
+ It is common practice across many open source projects using Git
+ to include a one or several “Signed-off-by” tags (generated by ‘git
+ commit -s’).
+ If the change has several authors, you are encouraged to use the
+ ‘Co-authored-by’ tag.
+ Relate tickets, tasks and bug issues are pointed in the commit message using
+ the JIRA tag.
+
+Files formatting
+~~~~~~~~~~~~~~~~
+
+**Files posted for review should use the UNIX/linux file format.**
+This means that their line terminator is “\\n” aka LF or LineFeed.
+**Other format such as MSDOS (with “\\r\n” aka CRLF aka Carriage Return Line
+Feed terminators) should be avoided.**
+Encoding formats commonly accepted are Unicode and ASCII.
+
+You can use the “file” linux command to check the actual status of your
+files.
+
+.. code-block:: text
+
+ $ file *
+ activate-projects-rtd-branch.sh: Bourne-Again shell script, ASCII text executable
+ branch-cutting-checklist.txt: ASCII text
+ ci-requirements.txt: ASCII text
+ docs: directory
+ find_bad_words.sh: ASCII text
+ INFO.yaml: ASCII text
+ README.md: ASCII text
+ tox.ini: ASCII text
+ web: directory
+
+
+and combine it with find and xargs + grep to detect MSDOS file
+
+.. code-block:: text
+
+ $ find . | xargs file | grep CRLF
+ ./tox.ini: ASCII text, with CRLF line terminators
+ ./docs/make.bat: DOS batch file, ASCII text, with CRLF line terminators
+
+
+then create a script with sed to remove the “\\r” special character and
+convert them in the UNIX format.
+
+.. code-block:: text
+
+ $ find . | xargs file | grep CRLF |grep -v make.bat | cut -d’:’ -f1 | xargs sed -i 's/\\r//'
+
+More easily, the vim editor can convert MSDOS file to UNIX format with
+‘:set ff=unix’ If you are on windows, do not use notepad since it only
+uses the MSDOS format. Consider using textpad++ or another advanced
+editor.
+
+**The ODL Java style guide limits the Java files line length to 120
+characters and 72 or 80 chars for javadoc.** It prohibits also the use
+of tabs. They should be replaced with 4 whitespaces. Below is a shell
+script to automate the operation inside a folder.
+
+.. code-block:: text
+
+ $ for file in * ; do sed -i 's/\\t/ /g' $file; done
+
+Trailing blanks should be avoided too. Below is a shell script to remove
+trailing whitespaces inside a folder.
+
+.. code-block:: text
+
+ $ for file in * ; do sed -i 's/ \*//' $file;done
+
+Useless empty lines must also be avoided.
+
+.. note::
+ If you are using an operating system with another default version of sed than
+ GNU sed, for example BSD sed on MAC OS X, you must adapt the examples given
+ here since the -i option takes a mandatory parameter.
+
+License issues
+~~~~~~~~~~~~~~
+
+The EPL license or a compatible license should be present on all
+projects code file in the header. The maven java checkstyle plugin will
+check the presence of this license.
+
+More details in the :ref:`coding-guidelines`.
+
+License issues are considered particularly sensible by the opensource
+communities.
+
Code Review / docs.git / commitdiff