Standardized code formatting using black

[wenkokke]

There are many formatting inconsistencies in the repository today. For instance, there are several files which inconsistently use tabs or spaces, which changes these layout in unpredictable ways, and several files have trailing white spaces, which may lead to horrible merge conflicts if they are ever removed by an editor.

There are two possible ways of setting this up:

• Set up a GitHub action which formats the code using black and commits it; or
• Set up a commit hook which formats the code using black, and set up a GitHub action which tests whether the formatting is consistent with black.

I'm creating this issue to document the need for a code formatter:

[rntz]

Black only formats python code, and I think the tab/space inconsistencies we saw were in talon files, and the excess whitespace issues we saw were in the README. I don't think a talon autoformatter exists, although it might not be too hard to make one. Not sure about markdown.

Is there a tool that fixes whitespace issues in general (whitespace-only lines, trailing newlines, etc)? That might serve us better than (or in addition to) a code formatter.

I also specifically dislike Black's approach to vertical spacing: it puts double newlines between top-level declarations, which I feel wastes vertical space and makes code harder to read, and also wastes lines on nothing but parentheses sometimes. Other options are autopep8 and yapf. https://www.kevinpeters.net/auto-formatters-for-python gives some examples of the behavior of each. I don't have much experience but think I would prefer autopep8 since it seems to be fairly minimal and not waste vertical space.

[rntz]

Hm, looks like autopep8 ALSO wants two blank lines between top-level definitions, as this is in the PEP8 style guide for python. Ugh. It does appear that you can configure it not to do this, though.

[wenkokke]

Black only formats python code, and I think the tab/space inconsistencies we saw were in talon files, and the excess whitespace issues we saw were in the README. I don't think a talon autoformatter exists, although it might not be too hard to make one. Not sure about markdown.

It should not be too hard to set up a similar process for tab conversion and whitespace trimming, in it would definitely be worth the effort in my opinion.

I also specifically dislike Black's approach to vertical spacing: it puts double newlines between top-level declarations, which I feel wastes vertical space and makes code harder to read, and also wastes lines on nothing but parentheses sometimes. Other options are autopep8 and yapf. https://www.kevinpeters.net/auto-formatters-for-python gives some examples of the behavior of each. I don't have much experience but think I would prefer autopep8 since it seems to be fairly minimal and not waste vertical space.

Personally, I am a big fan of the two black lines between top level definitions, as it makes code much easier for me to read.

[phillco]

Another useful tool I would recommend is flynt. It converts older styles of python strings to the newer f-strings, which I think are generally preferred. This would be separate from black which doesn't do this conversion last I checked.

flake8 could also be useful to add down the line. In my experience, it's good for catching things that would otherwise be runtime errors, like a forgotten import or misnamed variable references. But it would probably take some time to cleanup existing violations, and it also takes a little bit of configuration to work well with Black's rules.

Also, pre-commit is useful way for specifying all of the different formatters or linters to be used, and when they should be triggered (e.g. which file paths), and putting behind then behind a single entry point. The developer can run them locally with a single command, or add a local pre-commit hook to do so. You can also include it in CI of course. (There may be newer/better tools here, though)

[wenkokke]

Also, pre-commit is useful way for specifying all of the different formatters or linters to be used, and when they should be triggered (e.g. which file paths), and lets the developer run them locally and/or add a local pre-commit hook to do so at their leisure. You can also include it in CI of course. (There may be newer/better tools here, though)

While pre-commit hooks are very useful, for project such as this it is important to keep them portable. Which probably means ensuring that they support Windows without too much effort. Hence my slight preference for GitHub actions.

[phillco]

While pre-commit hooks are very useful, for project such as this it is important to keep them portable. Which probably means ensuring that they support Windows without too much effort. Hence my slight preference for GitHub actions.

I think pre-commit is just written in python, so it should be usable in Windows. But I haven't tried personally.

Also, despite the name, pre-commit itself is just a way to configure and dispatch multiple formatters in one place. Should run inside Actions just as easily as black itself does.

[wenkokke]

Git pre-commit hooks can be written in anything and are generally executed from the shell upon commit. Are you talking about something else?

[phillco]

Yes, I'm referring to the pre-commit library, which lets you write a YAML configuration that specifies all of the different linters, formatters, and other rules you want to run. You can then have it run all of the appropriate ones for your locally changed files by running pre-commit run, or run pre-commit run --all-files to run everything.

You can then put this command inside of a pre-commit hook or CI script, but this is optional. That's why I think it's misnamed.

The value is that as you add more steps, the developer doesn't need to learn/remember to run the different formatters. For example, if we were to write a reformatter for talon files, we could simply added to this configuration to run when .talon files are changed, and the developer would not need to remember to run both black and this other command.

I think your argument of using Actions achieves much the same, but I think it's nice to always be able to do so locally or if you don't want to push to GitHub.

[wenkokke]

It might be a good idea to vendor a specific version of the pre-commit library and any formatters we may want to use which are compatible with the version of Python that ships with Talon, and then to set it up to use Talon's Python either on commit or by using a voice command?

[auscompgeek]

On tab/space inconsistencies in .talon files, we could add an .editorconfig file? That'd help with anyone who uses an editor that respects EditorConfig.

On more Python formatters, there's also shed which runs a bunch of different formatters in sequence, including black. We probably don't need all of them though.

Vendoring the pre-commit tool would be non-trivial with the dependencies it has (pyyaml is definitely non-trivial). If we decide to use the pre-commit tool, vendoring any formatters would be counterproductive, since it's much more idiomatic to pin the versions you want in the pre-commit config.

I've noticed that the author of pre-commit has also spun up pre-commit.ci, a CI service specifically to run pre-commit. Looks like some prominent parts of the Python ecosystem use it, and apparently it auto-applies fixes to PRs. We don't have to use it of course, it runs fine in GitHub Actions.

[wenkokke]

I’ve noticed there are several files which break upon formatting with black. Unfortunately, I didn’t take note at the time, but we may want to be careful.

[auscompgeek]

Apparently black was run a while back, but seems it was never enforced: #171

[wenkokke]

Yes, the point of this issue is to start enforcing running black as part of the PR and commit process.

[phillco]

I've set this up in https://github.com/phillco/talon-axkit and it's worked well; you can integrate https://pre-commit.ci/, which is a free-for-OSS service which will push up formatting fixes to PR branches automatically, so that people don't have to install locally (you can see it here: phillco/talon-axkit#14).

And, pre-commit also allows you to install/run everything locally, and optionally create a pre-commit git hook if that's your preference (personally, I don't, it creates too much friction during development, so I run when I'm ready to submit).

Since black looks to be a bit controversial at the present, and also because we might want to do this in stages to make peoples' fork-merging more incremental, I'll submit a PR to add the non-controversial formatters for now:

• isort (sort formatters)
• flynt (convert older older string interpolation methods to fstrings)
• Other parts of shed that wouldn't be controversial
• Trimming trailing whitespace and converting tabs to spaces (all files including .talon files)

I'd be neat to write our own Talonscript linters/formatters down the line.

[phillco]

I wrote up a proposal for how to go about this: #816 and proposed the first PR: #817

[phillco]

Just cross posting my update in #816 here, in case people are not subscribed to that issue:

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.

[rntz]

@wenkokke We're now using black and a few other auto-formatting/prettification passes by default, integrated with github. Do you feel this issue is resolved/have further suggestions?

[wenkokke]

Amazing!

[phillco]

Also just cross-quoting @pokey's tip from the other issue in case anyone is not subscribed to it:

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

I've updated the description of #864 to keep a pinned ":bulb: how to deal with merge conflicts" section at the top and will link any other advice people have.