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