Extract recommendations how to craft changes into a separate doc page
dev-contributing.txt should focus on describing the contribution processes. At the moment we only have a single contribution process (push a change and wait for review), but we plan to establish a new contribution process with mentor guidance in addition. This new process should be described on dev-contributing.txt too. All recommendations how to craft changes are generally applicable, independent of which contribution process is chosen, hence it makes sense to have them on a separate page. Signed-off-by: Edwin Kempin <ekempin@google.com> Change-Id: Ia08da4ccd929c078761efd6ffe87a8883cc95118
This commit is contained in:
Edwin Kempin
@@ -23,6 +23,7 @@ Gerrit is developed as a self-hosting open source project:
|
|||||||
* link:dev-cla.html[Contributor License Agreement]
|
* link:dev-cla.html[Contributor License Agreement]
|
||||||
* link:dev-contributing.html[Contributing to Gerrit]
|
* link:dev-contributing.html[Contributing to Gerrit]
|
||||||
* link:dev-readme.html[Developer Setup]
|
* link:dev-readme.html[Developer Setup]
|
||||||
|
* link:dev-crafting-changes.html[Crafting Changes]
|
||||||
|
|
||||||
[[plugin-development]]
|
[[plugin-development]]
|
||||||
== Plugin Development
|
== Plugin Development
|
||||||
|
|||||||
@@ -11,9 +11,9 @@ As Gerrit is a code review tool, naturally contributions will
|
|||||||
be reviewed before they will get submitted to the code base. To
|
be reviewed before they will get submitted to the code base. To
|
||||||
start your contribution, please make a git commit and upload it
|
start your contribution, please make a git commit and upload it
|
||||||
for review to the main Gerrit review server. To help speed up the
|
for review to the main Gerrit review server. To help speed up the
|
||||||
review of your change, review these guidelines before submitting
|
review of your change, review these link:dev-crafting-changes.html[
|
||||||
your change. You can view the pending Gerrit contributions and
|
guidelines] before submitting your change. You can view the pending
|
||||||
their statuses
|
Gerrit contributions and their statuses
|
||||||
link:https://gerrit-review.googlesource.com/#/q/status:open+project:gerrit[here].
|
link:https://gerrit-review.googlesource.com/#/q/status:open+project:gerrit[here].
|
||||||
|
|
||||||
Depending on the size of that list it might take a while for
|
Depending on the size of that list it might take a while for
|
||||||
@@ -37,271 +37,6 @@ Reviewers don't want to seem like nags and pester you if you
|
|||||||
haven't replied or made a fix, so it helps them know if you
|
haven't replied or made a fix, so it helps them know if you
|
||||||
missed it or decided against it.
|
missed it or decided against it.
|
||||||
|
|
||||||
|
|
||||||
== Review Criteria
|
|
||||||
|
|
||||||
Here are some hints as to what approvers may be looking for
|
|
||||||
before approving or submitting changes to the Gerrit project.
|
|
||||||
Let's start with the simple nit picky stuff. You are likely
|
|
||||||
excited that your code works; help us share your excitement
|
|
||||||
by not distracting us with the simple stuff. Thanks to Gerrit,
|
|
||||||
problems are often highlighted and we find it hard to look
|
|
||||||
beyond simple spacing issues. Blame it on our short attention
|
|
||||||
spans, we really do want your code.
|
|
||||||
|
|
||||||
|
|
||||||
[[commit-message]]
|
|
||||||
=== Commit Message
|
|
||||||
|
|
||||||
It is essential to have a good commit message if you want your
|
|
||||||
change to be reviewed.
|
|
||||||
|
|
||||||
* 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
|
|
||||||
|
|
||||||
=== Setting up Vim for Git commit message
|
|
||||||
|
|
||||||
Git uses Vim as the default commit message editor. Put this into your
|
|
||||||
`$HOME/.vimrc` file to configure Vim for Git commit message formatting
|
|
||||||
and writing:
|
|
||||||
|
|
||||||
====
|
|
||||||
" Enable spell checking, which is not on by default for commit messages.
|
|
||||||
au FileType gitcommit setlocal spell
|
|
||||||
|
|
||||||
" Reset textwidth if you've previously overridden it.
|
|
||||||
au FileType gitcommit setlocal textwidth=72
|
|
||||||
====
|
|
||||||
|
|
||||||
|
|
||||||
[[git_commit_settings]]
|
|
||||||
=== A sample good Gerrit commit message:
|
|
||||||
====
|
|
||||||
Add sample commit message to guidelines doc
|
|
||||||
|
|
||||||
The original patch set for the contributing guidelines doc did not
|
|
||||||
include a sample commit message, this new patchset does. Hopefully this
|
|
||||||
makes things a bit clearer since examples can sometimes help when
|
|
||||||
explanations don't.
|
|
||||||
|
|
||||||
Note that the body of this commit message can be several paragraphs, and
|
|
||||||
that I word wrap it at 72 characters. Also note that I keep the summary
|
|
||||||
line under 50 characters since it is often truncated by tools which
|
|
||||||
display just the git summary.
|
|
||||||
|
|
||||||
Bug: Issue 98765605
|
|
||||||
Change-Id: Ic4a7c07eeb98cdeaf44e9d231a65a51f3fceae52
|
|
||||||
====
|
|
||||||
|
|
||||||
The `Change-Id` line is, as usual, created by a local git hook. To install it,
|
|
||||||
simply copy it from the checkout and make it executable:
|
|
||||||
|
|
||||||
====
|
|
||||||
cp ./gerrit-server/src/main/resources/com/google/gerrit/server/tools/root/hooks/commit-msg .git/hooks/
|
|
||||||
chmod +x .git/hooks/commit-msg
|
|
||||||
====
|
|
||||||
|
|
||||||
If you are working on core plugins, you will also need to install the
|
|
||||||
same hook in the submodules:
|
|
||||||
|
|
||||||
====
|
|
||||||
export hook=$(pwd)/.git/hooks/commit-msg
|
|
||||||
git submodule foreach 'cp -p "$hook" "$(git rev-parse --git-dir)/hooks/"'
|
|
||||||
====
|
|
||||||
|
|
||||||
|
|
||||||
To set up git's remote for easy pushing, run the following:
|
|
||||||
|
|
||||||
====
|
|
||||||
git remote add gerrit https://gerrit.googlesource.com/gerrit
|
|
||||||
====
|
|
||||||
|
|
||||||
The HTTPS access requires proper username and password; this can be obtained
|
|
||||||
by clicking the 'Obtain Password' link on the
|
|
||||||
link:https://gerrit-review.googlesource.com/#/settings/http-password[HTTP
|
|
||||||
Password tab of the user settings page].
|
|
||||||
|
|
||||||
[[style]]
|
|
||||||
=== Style
|
|
||||||
|
|
||||||
This project has a policy of Eclipse's warning free code. Eclipse
|
|
||||||
configuration is added to git and we expect the changes to be
|
|
||||||
warnings free.
|
|
||||||
|
|
||||||
We do not ask you to use Eclipse for editing, obviously. We do ask you
|
|
||||||
to provide Eclipse's warning free patches only. If for some reasons, you
|
|
||||||
are not able to set up Eclipse and verify, that your patch hasn't
|
|
||||||
introduced any new Eclipse warnings, mention this in a comment to your
|
|
||||||
change, so that reviewers will do it for you. Yes, the way to go is to
|
|
||||||
extend gerrit CI to take care of this, but it's not yet implemented.
|
|
||||||
|
|
||||||
Gerrit generally follows the
|
|
||||||
link:https://google.github.io/styleguide/javaguide.html[Google Java Style
|
|
||||||
Guide].
|
|
||||||
|
|
||||||
To format Java source code, Gerrit uses the
|
|
||||||
link:https://github.com/google/google-java-format[`google-java-format`]
|
|
||||||
tool (version 1.7), and to format Bazel BUILD, WORKSPACE and .bzl files the
|
|
||||||
link:https://github.com/bazelbuild/buildtools/tree/master/buildifier[`buildifier`]
|
|
||||||
tool (version 0.20.0).
|
|
||||||
These tools automatically apply format according to the style guides; this
|
|
||||||
streamlines code review by reducing the need for time-consuming, tedious,
|
|
||||||
and contentious discussions about trivial issues like whitespace.
|
|
||||||
|
|
||||||
You may download and run `google-java-format` on your own, or you may
|
|
||||||
run `./tools/setup_gjf.sh` to download a local copy and set up a
|
|
||||||
wrapper script. If you run your own copy, please use the same version,
|
|
||||||
as there may be slight differences between versions.
|
|
||||||
|
|
||||||
When considering the style beyond just formatting rules, it is often
|
|
||||||
more important to match the style of the nearby code which you are
|
|
||||||
modifying than it is to match the style guide exactly. This is
|
|
||||||
especially true within the same file.
|
|
||||||
|
|
||||||
Additionally, you will notice that most of the newline spacing
|
|
||||||
is fairly consistent throughout the code in Gerrit, it helps to
|
|
||||||
stick to the blank line conventions. Here are some specific
|
|
||||||
examples:
|
|
||||||
|
|
||||||
* Keep a blank line between all class and method declarations.
|
|
||||||
* Do not add blank lines at the beginning or end of class/methods.
|
|
||||||
|
|
||||||
When to use `final` modifier and when not (in new code):
|
|
||||||
|
|
||||||
Always:
|
|
||||||
|
|
||||||
* 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
|
|
||||||
|
|
||||||
Optional:
|
|
||||||
|
|
||||||
* final classes: use when appropriate, e.g. API restriction
|
|
||||||
* final methods: similar to final classes
|
|
||||||
|
|
||||||
Never:
|
|
||||||
|
|
||||||
* 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
|
|
||||||
|
|
||||||
=== Code Organization
|
|
||||||
|
|
||||||
Do your best to organize classes and methods in a logical way.
|
|
||||||
Here are some guidelines that Gerrit uses:
|
|
||||||
|
|
||||||
* 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.
|
|
||||||
|
|
||||||
Wow that's a lot! But don't worry, you'll get the habit and most
|
|
||||||
of the code is organized this way already; so if you pay attention
|
|
||||||
to the class you are editing you will likely pick up on it.
|
|
||||||
Naturally new classes are a little harder; you may want to come
|
|
||||||
back and consult this section when creating them.
|
|
||||||
|
|
||||||
|
|
||||||
=== Design
|
|
||||||
|
|
||||||
Here are some design level objectives that you should keep in mind
|
|
||||||
when coding:
|
|
||||||
|
|
||||||
* 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 Git repositories, including NoteDb.
|
|
||||||
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.
|
|
||||||
* ...and so is Guava (previously known as Google Collections).
|
|
||||||
|
|
||||||
|
|
||||||
=== Tests
|
|
||||||
|
|
||||||
* Tests for new code will greatly help your change get approved.
|
|
||||||
|
|
||||||
|
|
||||||
=== Change Size/Number of Files Touched
|
|
||||||
|
|
||||||
And finally, I probably cannot say enough about change sizes.
|
|
||||||
Generally, smaller is better, hopefully within reason. Do try to
|
|
||||||
keep things which will be confusing on their own together,
|
|
||||||
especially if changing one without the other will break something!
|
|
||||||
|
|
||||||
* 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.
|
|
||||||
|
|
||||||
== Finding starter projects to work on
|
== Finding starter projects to work on
|
||||||
|
|
||||||
We have created a
|
We have created a
|
||||||
|
|||||||
271
Documentation/dev-crafting-changes.txt
Normal file
271
Documentation/dev-crafting-changes.txt
Normal file
@@ -0,0 +1,271 @@
|
|||||||
|
= Gerrit Code Review - Crafting Changes
|
||||||
|
|
||||||
|
Here are some hints as to what approvers may be looking for
|
||||||
|
before approving or submitting changes to the Gerrit project.
|
||||||
|
Let's start with the simple nit picky stuff. You are likely
|
||||||
|
excited that your code works; help us share your excitement
|
||||||
|
by not distracting us with the simple stuff. Thanks to Gerrit,
|
||||||
|
problems are often highlighted and we find it hard to look
|
||||||
|
beyond simple spacing issues. Blame it on our short attention
|
||||||
|
spans, we really do want your code.
|
||||||
|
|
||||||
|
[[commit-message]]
|
||||||
|
== Commit Message
|
||||||
|
|
||||||
|
It is essential to have a good commit message if you want your
|
||||||
|
change to be reviewed.
|
||||||
|
|
||||||
|
* 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
|
||||||
|
|
||||||
|
[[vim-setup]]
|
||||||
|
=== Setting up Vim for Git commit message
|
||||||
|
|
||||||
|
Git uses Vim as the default commit message editor. Put this into your
|
||||||
|
`$HOME/.vimrc` file to configure Vim for Git commit message formatting
|
||||||
|
and writing:
|
||||||
|
|
||||||
|
====
|
||||||
|
" Enable spell checking, which is not on by default for commit messages.
|
||||||
|
au FileType gitcommit setlocal spell
|
||||||
|
|
||||||
|
" Reset textwidth if you've previously overridden it.
|
||||||
|
au FileType gitcommit setlocal textwidth=72
|
||||||
|
====
|
||||||
|
|
||||||
|
|
||||||
|
[[git-commit-settings]]
|
||||||
|
=== A sample good Gerrit commit message:
|
||||||
|
====
|
||||||
|
Add sample commit message to guidelines doc
|
||||||
|
|
||||||
|
The original patch set for the contributing guidelines doc did not
|
||||||
|
include a sample commit message, this new patchset does. Hopefully this
|
||||||
|
makes things a bit clearer since examples can sometimes help when
|
||||||
|
explanations don't.
|
||||||
|
|
||||||
|
Note that the body of this commit message can be several paragraphs, and
|
||||||
|
that I word wrap it at 72 characters. Also note that I keep the summary
|
||||||
|
line under 50 characters since it is often truncated by tools which
|
||||||
|
display just the git summary.
|
||||||
|
|
||||||
|
Bug: Issue 98765605
|
||||||
|
Change-Id: Ic4a7c07eeb98cdeaf44e9d231a65a51f3fceae52
|
||||||
|
====
|
||||||
|
|
||||||
|
The `Change-Id` line is, as usual, created by a local git hook. To install it,
|
||||||
|
simply copy it from the checkout and make it executable:
|
||||||
|
|
||||||
|
====
|
||||||
|
cp ./gerrit-server/src/main/resources/com/google/gerrit/server/tools/root/hooks/commit-msg .git/hooks/
|
||||||
|
chmod +x .git/hooks/commit-msg
|
||||||
|
====
|
||||||
|
|
||||||
|
If you are working on core plugins, you will also need to install the
|
||||||
|
same hook in the submodules:
|
||||||
|
|
||||||
|
====
|
||||||
|
export hook=$(pwd)/.git/hooks/commit-msg
|
||||||
|
git submodule foreach 'cp -p "$hook" "$(git rev-parse --git-dir)/hooks/"'
|
||||||
|
====
|
||||||
|
|
||||||
|
|
||||||
|
To set up git's remote for easy pushing, run the following:
|
||||||
|
|
||||||
|
====
|
||||||
|
git remote add gerrit https://gerrit.googlesource.com/gerrit
|
||||||
|
====
|
||||||
|
|
||||||
|
The HTTPS access requires proper username and password; this can be obtained
|
||||||
|
by clicking the 'Obtain Password' link on the
|
||||||
|
link:https://gerrit-review.googlesource.com/#/settings/http-password[HTTP
|
||||||
|
Password tab of the user settings page].
|
||||||
|
|
||||||
|
[[style]]
|
||||||
|
== Style
|
||||||
|
|
||||||
|
This project has a policy of Eclipse's warning free code. Eclipse
|
||||||
|
configuration is added to git and we expect the changes to be
|
||||||
|
warnings free.
|
||||||
|
|
||||||
|
We do not ask you to use Eclipse for editing, obviously. We do ask you
|
||||||
|
to provide Eclipse's warning free patches only. If for some reasons, you
|
||||||
|
are not able to set up Eclipse and verify, that your patch hasn't
|
||||||
|
introduced any new Eclipse warnings, mention this in a comment to your
|
||||||
|
change, so that reviewers will do it for you. Yes, the way to go is to
|
||||||
|
extend gerrit CI to take care of this, but it's not yet implemented.
|
||||||
|
|
||||||
|
Gerrit generally follows the
|
||||||
|
link:https://google.github.io/styleguide/javaguide.html[Google Java Style
|
||||||
|
Guide].
|
||||||
|
|
||||||
|
To format Java source code, Gerrit uses the
|
||||||
|
link:https://github.com/google/google-java-format[`google-java-format`]
|
||||||
|
tool (version 1.7), and to format Bazel BUILD, WORKSPACE and .bzl files the
|
||||||
|
link:https://github.com/bazelbuild/buildtools/tree/master/buildifier[`buildifier`]
|
||||||
|
tool (version 0.20.0).
|
||||||
|
These tools automatically apply format according to the style guides; this
|
||||||
|
streamlines code review by reducing the need for time-consuming, tedious,
|
||||||
|
and contentious discussions about trivial issues like whitespace.
|
||||||
|
|
||||||
|
You may download and run `google-java-format` on your own, or you may
|
||||||
|
run `./tools/setup_gjf.sh` to download a local copy and set up a
|
||||||
|
wrapper script. If you run your own copy, please use the same version,
|
||||||
|
as there may be slight differences between versions.
|
||||||
|
|
||||||
|
When considering the style beyond just formatting rules, it is often
|
||||||
|
more important to match the style of the nearby code which you are
|
||||||
|
modifying than it is to match the style guide exactly. This is
|
||||||
|
especially true within the same file.
|
||||||
|
|
||||||
|
Additionally, you will notice that most of the newline spacing
|
||||||
|
is fairly consistent throughout the code in Gerrit, it helps to
|
||||||
|
stick to the blank line conventions. Here are some specific
|
||||||
|
examples:
|
||||||
|
|
||||||
|
* Keep a blank line between all class and method declarations.
|
||||||
|
* Do not add blank lines at the beginning or end of class/methods.
|
||||||
|
|
||||||
|
When to use `final` modifier and when not (in new code):
|
||||||
|
|
||||||
|
Always:
|
||||||
|
|
||||||
|
* 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
|
||||||
|
|
||||||
|
Optional:
|
||||||
|
|
||||||
|
* final classes: use when appropriate, e.g. API restriction
|
||||||
|
* final methods: similar to final classes
|
||||||
|
|
||||||
|
Never:
|
||||||
|
|
||||||
|
* 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
|
||||||
|
|
||||||
|
[[code-organization]]
|
||||||
|
== Code Organization
|
||||||
|
|
||||||
|
Do your best to organize classes and methods in a logical way.
|
||||||
|
Here are some guidelines that Gerrit uses:
|
||||||
|
|
||||||
|
* 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.
|
||||||
|
|
||||||
|
Wow that's a lot! But don't worry, you'll get the habit and most
|
||||||
|
of the code is organized this way already; so if you pay attention
|
||||||
|
to the class you are editing you will likely pick up on it.
|
||||||
|
Naturally new classes are a little harder; you may want to come
|
||||||
|
back and consult this section when creating them.
|
||||||
|
|
||||||
|
[[design]]
|
||||||
|
== Design
|
||||||
|
|
||||||
|
Here are some design level objectives that you should keep in mind
|
||||||
|
when coding:
|
||||||
|
|
||||||
|
* 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 Git repositories, including NoteDb.
|
||||||
|
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.
|
||||||
|
* ...and so is Guava (previously known as Google Collections).
|
||||||
|
|
||||||
|
[[tests]]
|
||||||
|
== Tests
|
||||||
|
|
||||||
|
* Tests for new code will greatly help your change get approved.
|
||||||
|
|
||||||
|
[[change-size]]
|
||||||
|
== Change Size/Number of Files Touched
|
||||||
|
|
||||||
|
And finally, I probably cannot say enough about change sizes.
|
||||||
|
Generally, smaller is better, hopefully within reason. Do try to
|
||||||
|
keep things which will be confusing on their own together,
|
||||||
|
especially if changing one without the other will break something!
|
||||||
|
|
||||||
|
* 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.
|
||||||
|
|
||||||
|
GERRIT
|
||||||
|
------
|
||||||
|
Part of link:index.html[Gerrit Code Review]
|
||||||
|
|
||||||
|
SEARCHBOX
|
||||||
|
---------
|
||||||
Reference in New Issue
Block a user