I think I've seen something like +2 = "strong LGTM" and +1 = "weak LGTM; someone else should review" before. It's nice to have a shortcut which isn't a sentence when talking about weaker forms of LGTM.
On Sat, Jan 17, 2015 at 6:59 PM, <sandy.r...@cloudera.com> wrote: > I think clarifying these semantics is definitely worthwhile. Maybe this > complicates the process with additional terminology, but the way I've used > these has been: > > +1 - I think this is safe to merge and, barring objections from others, > would merge it immediately. > > LGTM - I have no concerns about this patch, but I don't necessarily feel > qualified to make a final call about it. The TM part acknowledges the > judgment as a little more subjective. > > I think having some concise way to express both of these is useful. > > -Sandy > > > On Jan 17, 2015, at 5:40 PM, Patrick Wendell <pwend...@gmail.com> wrote: > > > > Hey All, > > > > Just wanted to ping about a minor issue - but one that ends up having > > consequence given Spark's volume of reviews and commits. As much as > > possible, I think that we should try and gear towards "Google Style" > > LGTM on reviews. What I mean by this is that LGTM has the following > > semantics: > > > > "I know this code well, or I've looked at it close enough to feel > > confident it should be merged. If there are issues/bugs with this code > > later on, I feel confident I can help with them." > > > > Here is an alternative semantic: > > > > "Based on what I know about this part of the code, I don't see any > > show-stopper problems with this patch". > > > > The issue with the latter is that it ultimately erodes the > > significance of LGTM, since subsequent reviewers need to reason about > > what the person meant by saying LGTM. In contrast, having strong > > semantics around LGTM can help streamline reviews a lot, especially as > > reviewers get more experienced and gain trust from the comittership. > > > > There are several easy ways to give a more limited endorsement of a > patch: > > - "I'm not familiar with this code, but style, etc look good" (general > > endorsement) > > - "The build changes in this code LGTM, but I haven't reviewed the > > rest" (limited LGTM) > > > > If people are okay with this, I might add a short note on the wiki. > > I'm sending this e-mail first, though, to see whether anyone wants to > > express agreement or disagreement with this approach. > > > > - Patrick > > > > --------------------------------------------------------------------- > > To unsubscribe, e-mail: dev-unsubscr...@spark.apache.org > > For additional commands, e-mail: dev-h...@spark.apache.org > > > > --------------------------------------------------------------------- > To unsubscribe, e-mail: dev-unsubscr...@spark.apache.org > For additional commands, e-mail: dev-h...@spark.apache.org > >
