I think consistency is a good thing to strive for, but I think inconsistency is unavoidable, especially in an all-volunteer community and where each contribution is unique. I like Mike Walch's suggestion about how to clearly phrase things. I think recommended phrasing might make for good guidelines on how to perform a review.
On Mon, Jun 5, 2017 at 12:41 PM Mike Walch <mwa...@apache.org> wrote: > I agree with Christopher about avoiding strict guidelines for reviews but > having some loose guidelines/advice for reviewers. > > One thing that I have found helpful is when reviewers communicate how > strongly they feel about a change: > > - If the change is optional: Could add a unit test for this class > - If the change is suggested: A unit test should be added for this class > - If the change is required: I am -1 on this PR until a unit test is > added for this class. > > As a reviewer, I really like using "Could" before my PR comments. It lets > me suggest whatever change I want without forcing the contributor to make > these changes if they disagree. > > On Mon, Jun 5, 2017 at 12:00 PM Christopher <ctubb...@apache.org> wrote: > > > I'm not entirely sure what such specific guidelines hope to achieve. I'm > in > > favor of having some "things to look for" guidelines, but I don't think > > strict guidelines are going to work, because it elevates rules over the > > human element. I don't think we can be too prescriptive about what > things a > > contributor *should* do, or what a reviewer *should* check for, because > > everybody has unique volunteer time constraints, expertise, and passions. > > > > I'm also concerned about long-term maintenance of any such documentation, > > which seems likely to quickly get out of date as the community itself > > evolves, if we are too specific. Any such guidelines should be > maintainable > > by being succinct, generic, and flexible. I'm thinking something like: > > > > 1. Follow the ASF Code of Conduct [link] > > 2. Ensure build passes (captures most checks; can rely on Travis CI) > > [suggested build command-line] > > 3. Check for style and formatting failures, or uncommitted changes from > the > > build tooling after a build > > 5. Consider semver rules [link], especially when contributing to > > maintenance branches > > 6. Add tests when appropriate > > 7. Be willing to discuss performance impact, API design, style choices > > > > The key thing about contributing and reviewing is being willing to engage > > in polite discussion about any part of the contribution. Too much > > specificity will result in docs which don't apply to majority of > > circumstances, or docs which become out-of-date quickly, or docs which > > nobody reads because they are too complicated or long. The upfront costs > of > > trying to make sure you comply with all possible guidelines may also be a > > deterrence from contributing. It's much easier to be willing to receive > > friendly feedback than it is to try to make sure you take care of any > > possible thing somebody might result in a change suggestion. > > > > On Mon, Jun 5, 2017 at 11:25 AM Marc P. <marc.par...@gmail.com> wrote: > > > > > Dave, > > > I don't agree that stylistic changes are something to ignore. There > may > > > be cases where something is confusing to others and thus should be > called > > > out. This is difficult to blatantly avoid. > > > > > > I can't agree with number two either since a PR can be a form of > > > requirements elicitation and such there are cases in which there are > new > > > preconditions on the ticket. While your "not block of acceptance" may > > > sometimes apply I don't think it goes to fitting a community of > > developers, > > > where you can discuss your differences. In the case of number one and > two > > > developers reviewing will pick their battles and perhaps other > reviewers > > > can chime in on the importance of said feature. What is the purpose of > > > limiting this discussion my claiming it cannot impact acceptance? > > > > > > Bad code begets bad code and if a developer wants to take issue with > > > code, they should be allowed to discuss this within the PR. Further, > > > inconsistency begets inconsistency, so wild departures from the norm > > should > > > be something a reviewer has the levity to discuss. > > > > > > While discussion should lead to ticket creation we should avoid > > creating > > > features that need a portion completed to be used in production > > > successfully. > > > > > > > > > > > > > > > On Mon, Jun 5, 2017 at 11:08 AM, Dave Marion <dlmar...@comcast.net> > > wrote: > > > > > > > I propose that we define a set of guidelines to use when reviewing > pull > > > > requests. In doing so, contributors will be able to determine > potential > > > > issues in their code possibly reducing the number of changes that > occur > > > > before acceptance. Here's an example to start the discussion: > > > > > > > > > > > > Items a reviewer should look for: > > > > > > > > 1. Adherence to code formatting rules (link to formatting rules) > > > > > > > > 2. Unit tests required > > > > > > > > 3. Threading issues > > > > > > > > 4. Performance implications > > > > > > > > > > > > Items that should not block acceptance: > > > > > > > > 1. Stylistic changes that have no performance benefit > > > > > > > > 2. Addition of features outside the scope of the ticket (moving the > > goal > > > > post, discussion should lead to ticket creation) > > > > > > > > > >
