On Sat, Jan 2, 2016 at 11:13 PM, Ben Caradoc-Davies <b...@transient.nz> wrote:
> 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? > For the moment I suggest to keep it in the wiki, it's quicker to modify, but once the dust settles down some, why not, I believe it's the second time someone asks that very same question (to move it to the dev 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.) > I've tried to integrate the above in the guide. > > 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'm not aware of open source tools for Java that can be used in a differential manner, then ones I've seen are all or nothing, so you have to start with a "full clean" in order for them to point out misbehavior. Many of them are configurable, so if there is interest in working this, one could start by looking at just one rule (thinking FindBugs here), clean it up fully in the current code, and enable a build that would fail on subsequent violations. Normally these checks are pretty expensive to perform, so I'd advise to run them in a separate build (I've experience on some projects running PMD in each module and the build times are quite a bit higher, to the point that a "full build" is sort of a last resort measure, even on a fast machine). > > 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: > We already have one, which is also presented automatically by Gitbub when making a pull request, and thus referenceable as "Something you should have looked at before making a pull request": https://github.com/geoserver/geoserver/blob/master/CONTRIBUTING.md Just a though, maybe instead of the dev guide we could do a REVIEWING.md document right besides the CONTRIBUTING.md one, and then link them back to back. > - 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. > Good suggestions, want to add them to the above document? Cheers Andrea -- == GeoServer Professional Services from the experts! Visit http://goo.gl/it488V for more information. == *Geosolutions' Winter Holidays from 24/12 to 6/1* Ing. Andrea Aime @geowolf Technical Lead GeoSolutions S.A.S. Via Poggio alle Viti 1187 55054 Massarosa (LU) Italy phone: +39 0584 962313 fax: +39 0584 1660272 mob: +39 339 8844549 http://www.geo-solutions.it http://twitter.com/geosolutions_it *AVVERTENZE AI SENSI DEL D.Lgs. 196/2003* Le informazioni contenute in questo messaggio di posta elettronica e/o nel/i file/s allegato/i sono da considerarsi strettamente riservate. Il loro utilizzo è consentito esclusivamente al destinatario del messaggio, per le finalità indicate nel messaggio stesso. Qualora riceviate questo messaggio senza esserne il destinatario, Vi preghiamo cortesemente di darcene notizia via e-mail e di procedere alla distruzione del messaggio stesso, cancellandolo dal Vostro sistema. Conservare il messaggio stesso, divulgarlo anche in parte, distribuirlo ad altri soggetti, copiarlo, od utilizzarlo per finalità diverse, costituisce comportamento contrario ai principi dettati dal D.Lgs. 196/2003. The information in this message and/or attachments, is intended solely for the attention and use of the named addressee(s) and may be confidential or proprietary in nature or covered by the provisions of privacy act (Legislative Decree June, 30 2003, no.196 - Italy's New Data Protection Code).Any use not in accord with its purpose, any disclosure, reproduction, copying, distribution, or either dissemination, either whole or partial, is strictly forbidden except previous formal approval of the named addressee(s). If you are not the intended recipient, please contact immediately the sender by telephone, fax or e-mail and delete the information in this message that has been received in error. The sender does not give any warranty or accept liability as the content, accuracy or completeness of sent messages and accepts no responsibility for changes made after they were sent or for other risks which arise as a result of e-mail transmission, viruses, etc. -------------------------------------------------------
------------------------------------------------------------------------------
_______________________________________________ Geoserver-devel mailing list Geoserver-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/geoserver-devel
