On 02/01/16 23:59, Andrea Aime wrote: > I've put down in writing what I normally check during a full pull request > review. It's still a draft of course, additions/corrections (or discussion > about the contents) are welcomed: > https://github.com/geoserver/geoserver/wiki/Pull-request-review-guide
Andrea, that is very good. Will this document be moved to the developer guide? Other things I like to watch for: - Code style/formatting, no tabs, use of modern Java idioms including generics, foreach, and try-with-resources (which should reduce use of finally). I prefer JUnit 4 over JUnit 3. - Resource leaks: any time a close method is used outside a finally or try-with-resources block it attracts my attention. - Malicious or dangerous code from untrusted submitters. - static instances / singletons are a great way of introducing concurrency issues; you have this in your "Thread safety" section already but it might be worth mentioning that static instances are a red flag. (gt-app-schema DataAccessRegistry I am looking at you.) Do you use or recommend any linting tools that support differential operation, that is, reporting new suspect code practices introduced by a commit? Do you think such tools are helpful for this project? I also think that we need a pull request submitter guide. Reviews are less work when they are prepared with review in mind. Should pull request submitter guidelines be part of the same document? A few points I would like to see: - Architectural or widespread changes *must* first be discussed on the mailing lists. - Successful pull requests do more than just change code: they communicate intent to people. Pull requests are a conversation not a just a diff. Make your pull request as clear as possible to maximise your chances of acceptance. - Do not mix renaming/moving files or bulk reformatting with substantive changes as this can make pull requests very hard to understand. Put renaming/moving/bulk-reformatting in separate commits and let the reviewer know that you have done so. Do not squash commits if doing so mixes renaming/moving/bulk-reformatting with substantive changes. - Pull requests enable formal review by project leaders but also (1) make you learn more about your code by explaining it, and (2) let you teach the reviewer about your code. Review by a peer or junior or rubber duck is also valuable (although rubber ducks are not known for asking hard questions). Everyone will be able to see your pull request. Use this opportunity. Kind regards, -- Ben Caradoc-Davies <b...@transient.nz> Director Transient Software Limited <http://transient.nz/> New Zealand ------------------------------------------------------------------------------ _______________________________________________ Geoserver-devel mailing list Geoserver-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/geoserver-devel
