-
Notifications
You must be signed in to change notification settings - Fork 490
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Set expectations for coding style and code review in dev guide #3418
Comments
I recently listened to http://www.programmingthrowdown.com/2017/01/episode-62-php-and-hack.html where @MisterTea and his cohost Patrick really emphasized that though it takes some work for someone on the team to set up Checkstyle or whatever, it really pays off. They seem to have worked on large teams at big companies and the code style enforcement has always been in place. |
The conversation continues at #5075. |
Yesterday, @bmckinney @scolapasta @mheppler @djbrooke and I discussed the inconsistent formatting in the code base during an SBGrid sprint planning meeting. It was decided that some clarity on the expectations is warranted and that I would expand on what's currently at http://guides.dataverse.org/en/4.5.1/developers/coding-style.html
I'll also add to the dev guide somewhere the fantastic pro tip from Bill that for both individual commits and pull requests, you can add
?w=1
so that GitHub won't show you the whitespace changes in the "diff", making code review much easier if the diff contains hundreds of lines of only whitespace changes. (The GitHub web interface is the primary tool we use for code review.) For example, if you are tasked with reviewing https://github.com/IQSS/dataverse/pull/2422/files you have the equivalent of 20 pages to review but if you add?w=1
and look at https://github.com/IQSS/dataverse/pull/2422/files?w=1 you have only 8 pages to review! This is a game changer when it comes to the task of code review.I may or may not touch on the topic of git commit history. I'm personally interested in a clean git history ( https://www.reviewboard.org/docs/codebase/dev/git/clean-commits/ ) but I appreciate that many developers have an utter disregard for git commit history ( https://zachholman.com/posts/git-commit-history/ ). We should make all contributors feel welcome no matter which world view they hold.
Last night I played around with the Maven Checkstyle Plugin and there's a decent chance I'll add a bare-bones config that reports on tabs, which drives me the most crazy. It shouldn't be too tough to add the plaintext output of
mvn checkstyle:check
to our builds at https://travis-ci.org/IQSS/dataverseIn a way, this issue is a continuation of #775. Yesterday's conversation was a follow up to the "gofmt for Java" thread on the dataverse-dev mailing list: https://groups.google.com/d/msg/dataverse-dev/y2Jpk3szTf8/rckKmP6-BgAJ
Opinions and discussion are very welcome. We don't want to bikeshed too much on this but we do want to set and manage expectations to make the project as contributor-friendly as possible.
The text was updated successfully, but these errors were encountered: