Open Stack

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 06/16/2014 12:01 PM, Sean Dague wrote:
> On 06/16/2014 12:44 PM, Ben Nemec wrote:
>> On 06/16/2014 08:37 AM, Thierry Carrez wrote:
>>> Sean Dague wrote:
>>>> Hacking 0.9 series was released pretty late for Juno. The
>>>> entire check queue was flooded this morning with requirements
>>>> proposals failing pep8 because of it (so at 6am EST we were
>>>> waiting 1.5 hrs for a check node).
>>>> 
>>>> The previous soft policy with pep8 updates was that we set a 
>>>> pep8 version basically release week, and changes stopped
>>>> being done for style after first milestone.
>>>> 
>>>> I think in the spirit of that we should revert the hacking 
>>>> requirements update back to the 0.8 series for Juno. We're
>>>> past milestone 1, so shouldn't be working on style only fixes
>>>> at this point.
>>>> 
>>>> Proposed review here -
>>>> https://review.openstack.org/#/c/100231/
>>>> 
>>>> I also think in future hacking major releases need to happen 
>>>> within one week of release, or not at all for that series.
>> 
>>> We may also have reached a size where changing style rules is
>>> just too costly, whatever the moment in the cycle. I think it's
>>> good that we have rules to enforce a minimum of common style,
>>> but the added value of those extra rules is limited, while
>>> their impact on the common gate grows as we add more projects.
>> 
>> A few thoughts:
>> 
>> 1) I disagree with the proposition that hacking updates can only 
>> happen in the first week after release.  I get that there needs
>> to be a cutoff, but I don't think one week is reasonable.  Even
>> if we release in the first week, you're still going to be dealing
>> with hacking updates for the rest of the cycle as projects adopt
>> the new rules at their leisure.  I don't like retroactively
>> applying milestone 1 as a cutoff either, although I could see
>> making that the policy going forward.
>> 
>> 2) Given that most of the changes involved in fixing the new
>> failures are trivial, I think we should encourage combining the
>> fixes into one commit.  We _really_ don't need separate commits
>> to fix H305 and H307. This doesn't help much with the reviewer
>> load, but it should reduce the gate load somewhat.  It violates
>> the one change-one commit rule, but "A foolish consistency..."
> 
> The challenge is that hacking updates are basically giant merge
> conflict engines. If there is any significant amount of code
> outstanding in a project, landing hacking only changes basically
> means requiring much of the outstanding code to rebase.
> 
> So it's actually expensive in a way that doesn't jump out
> immediately. The cost of landing hacking isn't just the code of
> reviewing the hacking patches, it's also the cost of the extra
> roundtrips on outstanding patches.

If a project has that many violations of a new hacking rule then I'd
say they should just leave it disabled.  It already happens and I'd
rather have that than throw out a rule just because some projects
haven't been following it.

> 
>> 3) We should start requiring specs for all new hacking rules to
>> make sure we have consensus (I think oslo-specs is the place for
>> this).  2 +2's doesn't really accomplish that.  We also may need
>> to raise the bar for inclusion of new rules - while I agree with
>> all of the new ones added in hacking .9, I wonder if some of them
>> are really necessary.
> 
>> 4) I don't think we're at a point where we should freeze hacking 
>> completely, however.  The import grouping and long line wrapping 
>> checks in particular are things that reviewers have to enforce
>> today, and that has a significant, if less well-defined, cost
>> too.  If we're really going to say those rules can't be enforced
>> by hacking then we need to remove them from our hacking
>> guidelines and start the long process of educating reviewers to
>> stop requiring them.  I'd rather just deal with the pain of
>> adding them to hacking one time and never have to think about
>> them again.  I'm less convinced the other two that were added in
>> .9 are necessary, but in any case these are discussions that
>> should happen in spec reviews going forward.
> 
> I think both of those cases are really nits to the point that they 
> aren't worth enforcing. They won't change the correctness of the
> code. And barely change the readability.

Yeah, I suppose that's my personal dislike of \ line continuations
talking.

I don't like the idea of dropping our import guidelines though - I've
seen code that doesn't follow them and it's not nice to read.  Of
course, I'm horribly biased on that too since I wrote the check.  Then
again, it's something we've been enforcing manually all along, so in
theory it shouldn't be that difficult to fix either.

> 
> There are differences with things like the is None checks, or
> python 3 checks, which change correctness, or prevent subtle bugs.
> But I think we're now getting to a level of cleanliness enforcement
> that trumps functionally working.
> 
>> 5) We may want to come up with some way to globally disable pep8 
>> checks we don't want to enforce, since we don't have any control
>> over that but probably don't want to just stop updating pep8.
>> That could make the pain of these updates much less.
> 
> Actually, if you look at python novaclient, the doc string checks,
> which are really niggly, and part of hacking are the biggest fails.
> It's much less upstream that's doing this to us.

Sure, and I believe I raised that concern when the docstring check was
first proposed so I'm not going to argue too strongly for it.  I do
think it's a worthwhile thing if it's not too difficult to enable, but
if a project is going to have a huge number of docstrings to fix then
they just shouldn't enable the check.

> 
>> I could probably come up with a few more, but this is already
>> too wall-of-texty for my tastes. :-)
>> 
>> -Ben

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1
Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/

iQEcBAEBAgAGBQJTn1w9AAoJEDehGd0Fy7uqSpEH/2deuXIqVKFgwqJyw4Sde02l
khhikZUiUOgjggC8N3YxZHlCl7fSonZCuDrcKmIN5QtskZGgyrIiydJ2oavJsvdQ
jJhvJdDMbvOVU6UAQs0MoXTWSPjkFLLduQOv5aMpLKnxbGiQfCcc+R+rznixDdK3
EKtyVD6QMFG+PPVxy+catqqD8KA1S+YUa8mXK+keI23KTmflGY/CTf5x+fye9vsZ
MMv+hVL1RUqQIqh1HyJqMxzoTUinmLqxLZtbI1sqvYnOkHml/jt1ETRxPsB/S3Wk
sZUOb0PFwcI8FaauZbVqJ1m7K7Xm293IreXuD9CdyvmKOlGOk7jR3tkgxyVU3Gw=
=K2X9
-----END PGP SIGNATURE-----