Standardized formatting with pre-commit (plan)

[phillco]

This is a proposal for how to go about implementing #666. The thought here is to move incrementally to give everybody time to adapt to the new contribution flow, resolve conflicts with their branches, and/or identify problems caused by the reformatters.

I propose doing this in three phases, waiting a week or two in between each one:

Phase 1: Basic rules + CI enforcement

PR: #817

1. Add a basic .pre-commit-config.yaml with the most common/unobjectionable rules (standardizing trailing whitespace, consistent line endings, enforcing newline at the end of files, detecting unresolved merged conflict markers, etc.)
2. Add instructions to the README about how to use pre-commit.
3. To avoid merge conflicts, the PR will just introduce the config file -- a separate PR will show what the reformatting would look like, but it's not necessary to merge that PR or keep it up to date (see below).
4. After the configuration PR merges, a repository maintainer can run pre-commit run --all-files on master to apply all the fixes immediately to master and push them up.
5. After that, a repository maintainer can edit the branch protection rules on master to require a green pre-commit check for PRs.
6. Optionally, repository maintainers (either the main repo or forks) can connect their repositories to https://pre-commit.ci/. This is a free service for open-source repositories which automatically pushes formatting fixes to PR branches, if needed. This frees contributors from needing to run pre-commit locally if they are unable to or forget. We've been using this on AXKit and it's quite helpful (example).
7. Wait for the dust to settle, and for contributors to get used to the new flow and to resolve their merge conflicts.

Phase 2: Incremental parts of shed

shed is a "super all in one" reformatter+fixer for Python code, including the aggressive/opinionated black reformatter. It has the advantage of configuring these tools to work well together. Thanks to @auscompgeek for the recommendation.

In this phase, we add the "cleanup" parts of shed, but omit the massive reformatting of Black for now:

• autoflake, to remove unused imports and variables
• isort to sort imports, with autodetected first-party imports and --ca --profile=black args
• pyupgrade to upgrade older Python syntax
• flynt to convert older versions of string interpretation to f-strings (shed does not do this)

We'll also add prettier to reformat Markdown files -- this is optional if we want to cut down on the number of formatters, but nice to have.

As with the first step, we wait a little bit for the dust to settle and for problems to arise / merged conflicts to be resolved, etc.

Phase 3: Full Shed/Black

Finally, in this phase we apply shed in its entirety, which includes black. We can remove the individual utilities added in Phase 2 (except flynt), since shed includes them. We can set the --refactor, --py39-plus flags here now or do this as a final fourth phase.

Discussion

If people don't really think the phase 2/3 split is warranted, I'm happy to just combine them.

[rntz]

Haven't had time to look in detail yet, but it looks like some bits of shed do Python version detection. How can that possibly work? We want to match the version of Python that ships with talon public, which may not match any Python version installed on a dev's machine (even the one Talon uses if beta and public are on different versions).

[phillco]

Shed supports passing flags such as --py39-plus, so I assume that's how you override the detection. We would probably lock that at the Python version of Talon public.

But yeah, the user should probably install a matching version of Python -- I'll add that to the instructions.

[auscompgeek]

it looks like some bits of shed do Python version detection. How can that possibly work?

My understanding is that it mostly leans on black for this. It basically finds the newest syntax used in a file, and assumes you're targeting the corresponding Python version as a minimum, unless specified.

[pokey]

We might want to use .git-blame-ignore-revs for the commit that does the bulk reformat, as described here

[phillco]

Ah, great suggestion, thanks @pokey!

Also, just checked, looks like Talon Public uses Python 3.9.5, so we should be fine assuming 3.9 everywhere

[auscompgeek]

flynt to convert older versions of string interpretation to f-strings (shed does not do this)

pyupgrade does this too, but is conservative when facing ambiguity with %-formatting, specifically with a single %s and a bare variable RHS, IIUC. How many cases of these do we have (and would we expect to have in future contributions)?

[phillco]

I'll check!

[phillco]

Status update:

• @rntz and I merged pre-commit: check in basic rules #817, the first batch of changes basic formatters, and then pushed all of the formatting fixes to master.
• @knausj85 has set up https://pre-commit.ci/ which will automatically pushed formatting fixes to PR branches.
  • You can see this in action in README: add trailing space deliberately #820. I checked in formatting issue and it pushed a fix automatically.
• To tell git blame to ignore reformatting commits, @rntz checked in https://github.com/knausj85/knausj_talon/blob/master/.git-blame-ignore-revs.
  • You have to configure git to use this file by running git config --global blame.ignoreRevsFile .git-blame-ignore-revs.
  • Will add this to the README in a separate PR.

Merging in your forks

The reformatting will likely create some merge conflicts in your forks.

You can reduce the number by using the following git configuration when merging, which tells git to ignore whitespace differences when detecting conflicts:

$ git merge -Xignore-all-space upstream/master

(Relatedly, imerge is also a very useful tool for merging your fork incrementally, both @pokey and I have used it and can recommend it)

Please comment here if you find any other issues.

Conflict resolution offer

Lastly, I understand that these merged conflicts might be painful. To help speed the process and reduce pain, I'm making a standing offer to resolve the merge conflicts caused by auto formatting in your forks for you, if you would like.

The only caveats are that you need to give me access to your fork if it's private, and you also have to be merged up to the commit just before we introduced auto formatting (this offer only applies to the auto formatting itself :))

If you're interested, just reach out to me over Slack.

[phillco]

As far as moving on to the more aggressive formatters in phase 2/3, I think we want to wait a week or two to let the dust settle and see if any issues arise from people. If there are no problems, we proceed as planned :)

[pokey]

So shall we go ahead with phase 3 then?

[pokey]

Fwiw I just did an imerge to incorporate these formatting changes. I didn't use the -Xignore-all-space option, but what I did instead was the following:

Every time imerge presented me with a commit that conflicted with 2877a68, I would do the following:

1. Keep my version for every a merge conflict
2. Run pre-commit run --all-files
3. Stage and commit

This seemed to work well

[pokey]

Also, we may want to steal the setup from a personal project that I have. I based it off the Cursorless version, which went through fairly strenuous review, but had some fairly Cursorless-specific stuff in it. Here's the two files I think we could just steal verbatim:

• https://github.com/pokey/blind_files/blob/develop/.pre-commit-config.yaml
• https://github.com/pokey/blind_files/blob/develop/.editorconfig

[phillco]

Thanks @pokey! The only change I made was to set the max_line_length to 88 inside of .editorconfig since that's what black uses: https://github.com/knausj85/knausj_talon/pull/864/files

[phillco]

This should be complete; we manually merged #864: #864

[pokey]

Fwiw I created a gist showing how I did my merge https://gist.github.com/pokey/748ba17a2b7ee88c884992e477632ebb

[phillco]

Thanks @pokey, I added that in the description to #864 at the top, so hopefully people following the commit message find it :)