Excerpts from Jeremy Stanley's message of 2015-04-24 15:15:18 +0000: > On 2015-04-24 10:23:35 -0400 (-0400), Doug Hellmann wrote: > [...] > > I will often ask questions like, "what is going to happen in X > > situation if we change this default" or "how does this change in > > behavior affect the case where Y happens, which isn't well tested > > in our unit tests." If those details aren't made clear by the commit > > message and comments in the code, I consider that a good reason to > > include a -1 with a request for the author to provide more detail. > > Often these are cases I'm not intimately familiar with, so I ask a > > question rather than saying outright that I think something is > > broken because I expect to learn from the answer but I still have > > doubts that I want to indicate with the -1. > [...] > > Thinking about this though, for the benefit of non-fluent readers > and cultures where rhetorical questions are a less common construct, > it's probably better if we make a conscious effort to actually say > what we mean and give directly actionable feedback when reviewing. > > "Please add code comments here explaining the behavior in the case > where Y happens, since it isn't well tested in our unit tests." (Or > even better, "please add tests!") > > A lot of the good "questions" I see in reviews where there's a -1 > (and I'm frequently guilty of this too) could be much more > effectively phrased as requests to improve code comments, use > clearer syntax, add more detail in commit messages, or even better > test the code being added/changed.
That's a fair point. I tend to think of the questions as a way to start the discussion about the patch, rather than a subtle hint at what I think might be wrong. If that conversation reaches a point where more work is obviously needed, then I go ahead and make that request more plainly. Doug __________________________________________________________________________ OpenStack Development Mailing List (not for usage questions) Unsubscribe: [email protected]?subject:unsubscribe http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev
