# Fixing PEP8 technical debt at scale with sqre-codekit/autopep8

**URL:** <https://www.rubin.community/t/fixing-pep8-technical-debt-at-scale-with-sqre-codekit-autopep8/626>\
**Category:** Data Management\
**Created:** [March 27, 2016, 4:20am UTC](https://www.rubin.community/t/fixing-pep8-technical-debt-at-scale-with-sqre-codekit-autopep8/626 "2016-03-27T04:20:53Z")\
**Posts on this page:** 7\
**Page:** 1

<div class="post-metadata">

**Author:** ![jsick](https://sea2.discourse-cdn.com/flex002/user_avatar/www.rubin.community/jsick/32/672_2.png) [@jsick](https://www.rubin.community/u/jsick)\
**Post date:** [March 27, 2016, 4:20am UTC](https://www.rubin.community/t/fixing-pep8-technical-debt-at-scale-with-sqre-codekit-autopep8/626/1 "2016-03-27T04:20:53Z")

</div>

In [RFC-162](https://jira.lsstcorp.org/browse/RFC-162) we determined the set of PEP8 exclusions that are implied by our existing [DM Python Style Guide](http://developer.lsst.io/en/latest/coding/python_style_guide.html) (thanks @parejkoj, @timj and @frossie!). As @rowen noted, now that we can use linters in our editors for DM python code (and soon we’ll have [automated linting of pull requests](https://jira.lsstcorp.org/browse/DM-5637)), there is a large amount of technical debt associated with giving our codebase a clean PEP8 checkout. At this point, PEP8 fixes must be made before any new code can be merged.

[autopep8](https://github.com/hhatto/autopep8) gives us a way of fixing PEP8 issues against the DM Style Guide:

```bash
autopep8 . --in-place --recursive \
   --ignore E133,E226,E228,E251,N802,N803,W391 \
   --max-line-length 110

```

Given the large number of DM GitHub repos, we can take this automation one step further. In [sqre-codekit](https://github.com/lsst-sqre/sqre-codekit) we developed the infrastructure to clone all DM repos, make and commit code changes, and push those changes to a new branch from a single command. I’ve extended codekit to run autopep8:

```bash
lsst-autopep8 -u jonathansick --org lsst --team 'Data Management' \
    --branch tickets/DM-NNNN

```

(alternatively you can run `lsst-autopep8` for a single repo with a `--repo` argument).

See the code at [https://github.com/lsst-sqre/sqre-codekit/pull/3](https://github.com/lsst-sqre/sqre-codekit/pull/3).

An example of what autopep8 does to LSST repos can be found in the [https://github.com/jonathansick-shadow](https://github.com/jonathansick-shadow) test organization (see `u/jonathansick/autopep8` branches; here’s afw in particular: [https://github.com/jonathansick-shadow/afw/commit/660287ac88f56b5a8e0b506364b3845611f2962c](https://github.com/jonathansick-shadow/afw/commit/660287ac88f56b5a8e0b506364b3845611f2962c))

Given this:

1. Do we want to take an automated approach to fixing our PEP8 technical debt? If so,
2. What is the best way to coordinate the execution of `lsst-autopep8` so that there are as few merge conflicts as possible for those with existing tickets in these repos?

Note that `lsst-autopep8` only creates a new branch with PEP8 fixes; a manual merge step is still required. One can even imagine pushing additional formatting changes to the branch after autopep8 has done the initial work.

---

<div class="post-metadata">

**Author:** ![timj](https://sea2.discourse-cdn.com/flex002/user_avatar/www.rubin.community/timj/32/10_2.png) [@timj](https://www.rubin.community/u/timj)\
**Post date:** [March 27, 2016, 4:37am UTC](https://www.rubin.community/t/fixing-pep8-technical-debt-at-scale-with-sqre-codekit-autopep8/626/2 "2016-03-27T04:37:43Z")

</div>

It is tempting to take the opportunity to do the license fixes and `futurize -1` on the same timescale.

Coordinating the changes for minimal impact with existing ticket branches may be a thorny problem. Ideally we’d sneak it in at the end of one ticket branch and before someone else starts something. That might be feasible for some repos but not all. On the plus side, since it’s automated doing the fixes again on the few ticket branches before merging might be doable.

---

<div class="post-metadata">

**Author:** ![parejkoj](https://sea2.discourse-cdn.com/flex002/user_avatar/www.rubin.community/parejkoj/32/138_2.png) [@parejkoj](https://www.rubin.community/u/parejkoj)\
**Post date:** [March 27, 2016, 5:46am UTC](https://www.rubin.community/t/fixing-pep8-technical-debt-at-scale-with-sqre-codekit-autopep8/626/3 "2016-03-27T05:46:36Z")

</div>

> [@jsick](#):
>
> Do we want to take an automated approach to fixing our PEP8 technical debt?

Yes, yes, a thousand times yes.

> [@jsick](#):
>
> What is the best way to coordinate the execution of lsst-autopep8 so that there are as few merge conflicts as possible for those with existing tickets in these repos?

Is this another “wait until the HSC merge is complete”, thing?

---

<div class="post-metadata">

**Author:** ![timj](https://sea2.discourse-cdn.com/flex002/user_avatar/www.rubin.community/timj/32/10_2.png) [@timj](https://www.rubin.community/u/timj)\
**Post date:** [March 27, 2016, 5:54am UTC](https://www.rubin.community/t/fixing-pep8-technical-debt-at-scale-with-sqre-codekit-autopep8/626/4 "2016-03-27T05:54:53Z")

</div>

> [@parejkoj](#):
>
> Is this another “wait until the HSC merge is complete”, thing?

That’s pretty [much done](http://www.rubin.community/t/dm-highlights-week-of-2016-03-09-to-2016-03-15/603) so no worries there.

---

<div class="post-metadata">

**Author:** ![jsick](https://sea2.discourse-cdn.com/flex002/user_avatar/www.rubin.community/jsick/32/672_2.png) [@jsick](https://www.rubin.community/u/jsick)\
**Post date:** [March 27, 2016, 6:33am UTC](https://www.rubin.community/t/fixing-pep8-technical-debt-at-scale-with-sqre-codekit-autopep8/626/5 "2016-03-27T06:33:15Z")

</div>

Perhaps the best approach is

1. Poll everyone on what repos they’re actively developing for,
2. Run autopep8, RFC-45 licensing, and `futurize -1` on ‘dormant’ repos and get those merges in.
3. Work with primary developers on a case-by-case basis to get these changes made to the active repos. Certainly we could ask for autopep8 to be run at the end of ticket branches.
4. Have some sort of record keeping (ideally we can CI the PEP8/license stuff automatically) to ensure these changes make it by the end of the X2016 cycle.

---

<div class="post-metadata">

**Author:** ![jsick](https://sea2.discourse-cdn.com/flex002/user_avatar/www.rubin.community/jsick/32/672_2.png) [@jsick](https://www.rubin.community/u/jsick)\
**Post date:** [March 27, 2016, 7:19am UTC](https://www.rubin.community/t/fixing-pep8-technical-debt-at-scale-with-sqre-codekit-autopep8/626/6 "2016-03-27T07:19:00Z")

</div>

Brain-dump of other things that could benefit from a mass codebase edit cycle:

- Adding a CONTRIBUTING template
- Add setup.cfg with PEP8 exception list
- Add `doc/xml` to `.gitignore`

---

<div class="post-metadata">

**Author:** ![timj](https://sea2.discourse-cdn.com/flex002/user_avatar/www.rubin.community/timj/32/10_2.png) [@timj](https://www.rubin.community/u/timj)\
**Post date:** [March 27, 2016, 3:41pm UTC](https://www.rubin.community/t/fixing-pep8-technical-debt-at-scale-with-sqre-codekit-autopep8/626/7 "2016-03-27T15:41:00Z")

</div>

> [@jsick](#):
>
> W391

If we are going to bulk convert I’d probably just remove W391 from the ignore list as well. The main reason for adding it was not existing coding style but prevalence in code base.
