Enabling hacking extensions: The right way

Of late, I wake up every morning revving to go and work on the next cool thing in Trove and I see that overnight some well-meaning person has contributed a change that looks something like this:

String interpolation should be delayed to be handled by the logging code, rather than being done at the point of the logging call.
Ref:http://docs.openstack.org/developer/oslo.i18n/guidelines.html#log-translation For example:
# WRONG
LOG.info(_LI(‘some message: variable=%s’) % variable)
# RIGHT
LOG.info(_LI(‘some message: variable=%s’), variable)

And the code submitted fixes a small number (lets say 5) places where strings sent to logging are rendered.

As I said at the TC-Board meeting in Barcelona, these well-meaning people are actually submitting what on the face of it appear to be valid corrections to the code. Yet, I submit to you that these changes represent a scourge that we should stamp out.

I know for a fact that in Trove there are (currently) 751 occurrences of this particular style error. This is the hacking extension H904, and when enabled in Trove, I get this:

$ tox -e pep8 | grep H904 | wc -l
751

That’s the catch, Trove does not enable this hacking extension. A quick look indicates that only Neutron does.

Why are these well meaning changes a scourge? Here’s why …

  • They don’t materially improve a project to fix a small fraction of these errors without preventing them from reoccurring
  • Each of these changes takes some considerable CI resources to verify and get approved
  • Each of these changes take time for someone to review, time which could be better spent if we were to fix these problems properly.

So, I submit to you that if you want to submit a patch to fix one of these hacking issues, here is the right way. Of course, I’m opinionated, I’m going to reference one of my own changes as an example!

  1. If your project does not have hacking extensions, this commit shows you what you have to do to enable that. You may have to bump test-requirements.txt and update the version of hacking that you use in order to use the ‘enable-extensions’ option.
  2. Enable the hacking rule or extension for the particular style issue at hand; let’s illustrate with H203. H203 ensures that we use assertNone() and not assertEqual(None, …).
  3. Run the pep8 test against the project and find and correct all places where the failure occurs. Typically this is accomplished by just running ‘tox -e pep8’.
  4. Test that the code does in fact work as expected; correcting style guidelines can introduce functional errors so make sure that the unit tests pass too. Typically this is accomplished by running ‘tox -e py27 -e py34’.
  5. Actually exercise the system; launch a system with devstack and the project enabled, and actually exercise the system. In the case of Trove, actually build a guest and launch a database or two.
  6. Then submit your change including the change to tox.ini that enables the hacking rule for review.

Well, that’s a lot of work! Sure, you really have to work for your Stackalytics credit, right? I’m sure the load on the CI system will show that this is worthwhile.

It is better to do things this way in the long run. With the hacking rule enabled, future commits will also comply with the rule (they will fail pep8 if they don’t). And that will put an end to the cottage industry that has sprung up around finding these kinds of errors and fixing them one at a time.

In conclusion I urge reviewers in all projects to summarily reject style changes that don’t also enable a hacking rule. Approving them is the wrong thing to do. Require the contributor to enable the hacking rule, and fix the problem the right way. That’s what a good code review is about.

4 thoughts on “Enabling hacking extensions: The right way”

  1. Agreed, such style changes should come with testing enabled whenever possible. Just one caveat: With 750+ lines to change, it would take lots of rebasing due to merge conflicts if this is a one single large patch – or a very fast review of the core team.

    Instead, I would split it into smaller changes like all files in one sub-dir, or those starting with certain letters, submitt them one by one – and then have a final change that depends on all of them (using “Depends-On”) and enables the test. That will still have some rebases but those can be reviewed much better. They produce initially a higher load due on the CI system but that is nothing compared to the number of rebases you have to do. Small changes that are easy to review – with the promise to fix them all (that you’ve shown with submitting the whole series) plus adding the tests in the end would be my way to addressing your 750+ nits in trove. Who takes this up now?😉

    Like

    1. I wholeheartedly agree to your approach if we can get some commitment from the contributor that they will follow through with the whole project. So sure, many commits but submit them all at once and show me the one commit that has the hacking rule enabled with dependencies that show that you have gone all the way!

      Like

Leave a Reply

Fill in your details below or click an icon to log in:

WordPress.com Logo

You are commenting using your WordPress.com account. Log Out / Change )

Twitter picture

You are commenting using your Twitter account. Log Out / Change )

Facebook photo

You are commenting using your Facebook account. Log Out / Change )

Google+ photo

You are commenting using your Google+ account. Log Out / Change )

Connecting to %s