I agree with you Jim, but sadly it's a practice in the reality to -1 for
nitpicks or even for questions.
I'd prefer if that nitpicks can be flagged, but as a comment, so they
can be corrected in future iterations if the patch needs more work,
but not a blocking -1.
Also same as -1 for questions, it's really a problem sometimes, because
even if you answer the question, that -1 remains there until the people
who asked reviews again, and prevents changes to land.
I admit I also do that sometimes, because I have the perception that
if i just add a coment and not flag with -1 or +1, i get less attention
from the person being reviewed. We should find ways to solve it.
So i'm glad that this conversation raised, and that this improves us
on review quality and speed.
Best
Yolanda
El 12/08/15 a las 18:00, James E. Blair escribió:
Paul Belanger <[email protected]> writes:
...
However, recently. I got my hand smacked in 2 different code reviews for arrow
alignment issues. Honestly, I wasn't even mad about the -1 for the alignment.
However, I'm concerned about the wasted effort the -1 caused me. Basically, I
had to wait a few days to get the -1, since it was a human doing the review, not
the gate. Additionally, if I was getting a -1 for style checks, why didn't
jenkins do it?
To me, this is the crux of the problem. It's okay for us to have a
conversation about whether we want to change the lint rules for
system-config, but for the moment, we have chosen to have them
disabled (let's set that conversation aside for now).
Consider that when you leave a -1 on a code review, you are asking a
contributor to go back and re-do their work. You are also asking other
reviewers to re-review a change. It's a lot of work, and to do it
frivolously means we are wasting peoples' time. So, please, when you
think about whether to leave a -1, don't think "Aha! I found something
that could be improved!", instead think "I found something, but is it
worth redoing already completed work?"
In particular, many of us have a very strong aversion to nitpicking
about whitespace. In many cases, we are not in universal agreement on
what constitutes the most aesthetically pleasing indentation. In these
cases, we often loosen the enforcement of our tools so that a wider
variety of possibilities is accepted. If we've made such a choice,
consider embracing the philosophy of IDIC and accepting that there may
be more than one way to wrap a line.
I have proposed a change to system-config to document what we look for
in code-review, and what should and should not constitute a -1, so that
new folks have some background:
https://review.openstack.org/#/c/212073/
-Jim
_______________________________________________
OpenStack-Infra mailing list
[email protected]
http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-infra
--
Yolanda Robla Mota
Cloud Automation and Distribution Engineer
+34 605641639
[email protected]
_______________________________________________
OpenStack-Infra mailing list
[email protected]
http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-infra
