Niklas, Aaron S., Siebrand, Santhosh, Amir, Alolita had a discussion
about code review. Here are the notes. It might repeat some things
Sumana sent earlier and you might not agree on everything.
* Having tags in Gerrit would be nice. Commit summary lines are
sometimes abused for this.
* Changes and rebases are combined.
* We are concerned that dashboards will become private in future.
Currently it's possible to see everyone's dashboards.
* Time is wasted scanning for stuff to review in different Gerrit listings.
* Unowned code rotting around. Result of bad bus factor.
* Lack of tools to effectively debug PHP code without
* Not always testing manually. Depends on how big failure can be and
whether there are good unit tests for it.
* Drafts-feature is (sometimes) good for work-in-progress, but tests
need to be triggered manually by adding Jenkins to reviewers and
triggering new patchset.
* We own/watch certain extensions and review new patches against them.
* Sadly, automatic code formatting tools are rarely used.
* Post-commit review does not yet have a process, but cases seem to be
* We love the continous integration systems running our phpunit tests,
looking forward to QUnit tests too.
* Use wfProfilneIn/Out for things like reading or writing to files,
shell calls, http calls.
Our quick tips (what we usually complain in code review)
* Commit should message describe what and why. What was the problem?
How does the fix resolve it? How to test that it actually works?
* Commits should come with unit tests when possible.
* Make methods public/protected/private (think what makes sense).
Don't just make everything public!
* Follow coding style (whitespace is important).
* Spelling mistakes.
* Avoid long functions (100+ lines).
* Document all methods (even if protected) in general. Describe what
they do rather than documenting the obvious @param $title Title Title
* New public methods and classes should have @since tags. Extension
developers will love you.
* Avoid saying "patchset x: ..." in the commit summary, use gerrit
* Use type hinting when applicable
* If you need some additional functionality of a core module (or you
need a function that does something similar but a different), actually
improve the core module. Don't just copy+paste and modify the code *
in another place.
* Avoid static "inheritance" (late static binding) unless it's perhaps
for a factory function
* Smaller commits are easier to review
* Extensions that assume direct file system access for shared storage
can't be used on WMF
* Refactor code as changes are made (but use separate commits if the
refactoring is large). Don't let the code keep getting worse with each
It should be said, this is part of the recent "five tips to get your
code reviewed faster". You should never combine a rebase with an actual
substantial change, because it makes it very hard to compare between
patchsets. (I didn't see this in the "quick tips" list, thought I should
And if you can, try to use the "rebase" button as much as possible!
On Thu, Sep 20, 2012 at 3:42 PM, Mark Holmquist <[hidden email]> wrote:
>> * Changes and rebases are combined.
> It should be said, this is part of the recent "five tips to get your code
> reviewed faster". You should never combine a rebase with an actual
> substantial change, because it makes it very hard to compare between
> patchsets. (I didn't see this in the "quick tips" list, thought I should
> mention it)
> And if you can, try to use the "rebase" button as much as possible!
Couple of changes coming in Gerrit upstream that will make this better:
- Gerrit won't perform the rebase if it's not necessary
- Changes as a result of a rebase aren't shown in the changes list when
comparing to an old patchset
I believe the former made it into 2.5 (need to double check). Pretty sure
the latter didn't, unfortunately.
> - Gerrit won't perform the rebase if it's not necessary
Cool! I think the only reason this will be better is "reduced number of
patchsets", but that's a good thing nevertheless.
> - Changes as a result of a rebase aren't shown in the changes list when
> comparing to an old patchset
Hm. Will this be file-level whitelisting (i.e., "this file changed from
the master branch in this patchset, so we'll show the changes") or is it
line-level? If the latter, how? Because I'm not sure it's trivial....
Well, unless this is only applicable to Gerrit-performed rebases, and
won't be helpful for conflict-induced manual ones, which wouldn't be
nearly as useful.
On Thu, Sep 20, 2012 at 2:08 PM, Mark Holmquist <[hidden email]> wrote:
> Hm. Will this be file-level whitelisting (i.e., "this file changed from the
> master branch in this patchset, so we'll show the changes") or is it
> line-level? If the latter, how? Because I'm not sure it's trivial....
I believe it's file-level, which eliminates most but not all noise.