Code review meeting notes

classic Classic list List threaded Threaded
5 messages Options
Reply | Threaded
Open this post in threaded view
|

Code review meeting notes

Niklas Laxström
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.

Problems:
* 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.
* There is no stylize.php for JavaScript.
* Lack of tools to effectively debug PHP code without
print/var_dump/error_log etc.

Notes:
* 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.
* JavaScript code needs more documentation than PHP.
* Sadly, automatic code formatting tools are rarely used.
* Post-commit review does not yet have a process, but cases seem to be
relatively rare.
* 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
object.
* New public methods and classes should have @since tags. Extension
developers will love you.
* Avoid saying "patchset x: ..." in the commit summary, use gerrit
comments instead
* 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
change.

  -Niklas

--
Niklas Laxström

_______________________________________________
Wikitech-l mailing list
[hidden email]
https://lists.wikimedia.org/mailman/listinfo/wikitech-l
Reply | Threaded
Open this post in threaded view
|

Re: Code review meeting notes

Mark Holmquist-2
> * 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!

--
Mark Holmquist
Software Engineer, Wikimedia Foundation
[hidden email]
http://marktraceur.info

_______________________________________________
Wikitech-l mailing list
[hidden email]
https://lists.wikimedia.org/mailman/listinfo/wikitech-l
Reply | Threaded
Open this post in threaded view
|

Re: Code review meeting notes

Chad
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.

-Chad

_______________________________________________
Wikitech-l mailing list
[hidden email]
https://lists.wikimedia.org/mailman/listinfo/wikitech-l
Reply | Threaded
Open this post in threaded view
|

Re: Code review meeting notes

Mark Holmquist-2
> - 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.

--
Mark Holmquist
Software Engineer, Wikimedia Foundation
[hidden email]
http://marktraceur.info

_______________________________________________
Wikitech-l mailing list
[hidden email]
https://lists.wikimedia.org/mailman/listinfo/wikitech-l
Reply | Threaded
Open this post in threaded view
|

Re: Code review meeting notes

Roan Kattouw-2
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.

Roan

_______________________________________________
Wikitech-l mailing list
[hidden email]
https://lists.wikimedia.org/mailman/listinfo/wikitech-l