[OpenStack-Infra] Puppet lint checks for system-config
Paul Belanger
pabelanger at redhat.com
Wed Aug 12 16:51:30 UTC 2015
On Wed, Aug 12, 2015 at 09:00:10AM -0700, James E. Blair wrote:
> Paul Belanger <pabelanger at redhat.com> 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).
>
Ya, this was the initial reason for my post, to revisit the discussion.
> 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/
>
Thanks for doing this.
> -Jim
More information about the OpenStack-Infra
mailing list