Gerrit Code Review - Contributing

Keep lines no longer than 72 chars

Start with a short one line summary

Followed by a blank line

Followed by one or more explanatory paragraphs

Use the present tense (fix instead of fixed)

Use the past tense when describing the status before this commit

Include a Bug: Issue <#> line if fixing a Gerrit issue, or a Feature: Issue <#> line if implementing a feature request.

Include a Change-Id line

Keep a blank line between all class and method declarations.

Do not add blank lines at the beginning or end of class/methods.

final fields: marking fields as final forces them to be initialized in the constructor or at declaration

final static fields: clearly communicates the intent

to use final variables in inner anonymous classes

final classes: use when appropriate, e.g. API restriction

final methods: similar to final classes

local variables: it clutters the code, and makes the code less readable. When copying old code to new location, finals should be removed

method parameters: similar to local variables

Ensure a standard copyright header is included at the top of any new files (copy it from another file, update the year).

Always place loggers first in your class!

Define any static interfaces next in your class.

Define non static interfaces after static interfaces in your class.

Next you should define static types, static members, and static methods, in decreasing order of visibility (public to private).

Finally instance types, instance members, then constructors, and then instance methods.

Some common exceptions are private helper static methods, which might appear near the instance methods which they help (but may also appear at the top).

Getters and setters for the same instance field should usually be near each other barring a good reason not to.

If you are using assisted injection, the factory for your class should be before the instance members.

Annotations should go before language keywords (final, private, etc)
Example: @Assisted @Nullable final type varName

Prefer to open multiple AutoCloseable resources in the same try-with-resources block instead of nesting the try-with-resources blocks and increasing the indentation level more than necessary.

ORM entity objects should match exactly one row in the database.

Most client pages should perform only one RPC to load so as to keep latencies down. Exceptions would apply to RPCs which need to load large data sets if splitting them out will help the page load faster. Generally page loads are expected to complete in under 100ms. This will be the case for most operations, unless the data being fetched is not using Gerrit’s caching infrastructure. In these slower cases, it is worth considering mitigating this longer load by using a second RPC to fill in this data after the page is displayed (or alternatively it might be worth proposing caching this data).

@Inject should be used on constructors, not on fields. The current exceptions are the ssh commands, these were implemented earlier in Gerrit’s development. To stay consistent, new ssh commands should follow this older pattern; but eventually these should get converted to eliminate this exception.

Don’t leave repository objects (git or schema) open. A .close() after every open should be placed in a finally{} block.

Don’t leave UI components, which can cause new actions to occur, enabled during RPCs which update the DB. This is to prevent people from submitting actions more than once when operating on slow links. If the action buttons are disabled, they cannot be resubmitted and the user can see that Gerrit is still busy.

GWT EventBus is the new way forward.

…​and so is Guava (previously known as Google Collections).

Tests for new code will greatly help your change get approved.

If a new feature is implemented and it is a larger one, try to identify if it can be split into smaller logical features; when in doubt, err on the smaller side.

Separate bug fixes from feature improvements. The bug fix may be an easy candidate for approval and should not need to wait for new features to be approved. Also, combining the two makes reviewing harder since then there is no clear line between the fix and the feature.

Separate supporting refactoring from feature changes. If your new feature requires some refactoring, it helps to make the refactoring a separate change which your feature change depends on. This way, reviewers can easily review the refactor change as a something that should not alter the current functionality, and feel more confident they can more easily spot errors this way. Of course, it also makes it easier to test and locate later on if an unfortunate error does slip in. Lastly, by not having to see refactoring changes at the same time, it helps reviewers understand how your feature changes the current functionality.

Separate logical features into separate changes. This is often the hardest part. Here is an example: when adding a new ability, make separate changes for the UI and the ssh commands if possible.

Do only what the commit message describes. In other words, things which are not strictly related to the commit message shouldn’t be part of a change, even trivial things like externalizing a string somewhere or fixing a typo. This helps keep git blame more useful in the future and it also makes git revert more useful.

Use topics to link your separate changes together.

Update common and plugin dependencies in tools/gwt-constants.defs.

Update to the same GWT version in the cookbook plugin and optionally in other plugins that have a dependency on GWT.

Update the GWT version in the archetype metadata in the gerrit-plugin-gwt-archetype.

Update the version of gwt-maven-plugin in the example pom.xml file in dev-plugins.

Update to the same GWT version in the gwtjsonrpc project, and release a new version.