Thanks, Reuven, for bringing this up. This is an area well worth investing time in. Specific comments below.
On Mon, Feb 19, 2018 at 10:32 AM, Reuven Lax <re...@google.com> wrote: > Pedantic > > Overly-pedantic comments (change variable names, etc.) can be frustrating. > The PR author can feel like they are being forced to make meaningless > changes just so the reviewer will allow merging. Note that this is sometimes > in the eye of the beholder - the reviewer may not think all these comments > are pedantic. I think it would be good to have a convention for comments that are suggestive, but not required. I usually prefix these with "Nit: ..." Maybe "Optional: ..." would be better. This makes it clear that we're not trying to put up unnecessary burdens for getting code in, but on the other hand helps give guidance (which is often appreciated, especially for newcomers (to the project, or coding in general)). For any comment, one should be able to answer the question "why" and a reviewer should feel free to ask this. > Don't Do This > > Sometimes a reviewer rejects an entire PR, saying that this should not be > done. There are various reasons given: this won't scale, this will break > backwards compatibility, this will break a specific runner, etc. The PR > author may not always understand or agree with these reasons, and this can > leave hurt feelings. As mentioned, being able to point to a doc when issues like this arise is a much better experience for the recipient. I like the PR dashboard idea as well. A separate package for less-well-vetted code is better than an @Expiremental attribute. Incubating? Is there an expectation of graduation at some point?
