pre-commit: run all pre-commit autoformatting hooks, add one for ci/check-embedded-chart-code.py script as well and run it

jupyterhub / binderhub

Run your code in the cloud, with technology so advanced, it feels like magic!

https://binderhub.readthedocs.io

BSD 3-Clause "New" or "Revised" License

2.57k stars 390 forks source link

pre-commit: run all pre-commit autoformatting hooks, add one for ci/check-embedded-chart-code.py script as well and run it #1465

Closed consideRatio closed 2 years ago

consideRatio commented 2 years ago

This is a followup, running the autoformatting hooks defined in .pre-commit-config which was introduced in #1381.

It also includes a .git-blame-ignore-revs file with inline comments of what that is about as suggested by @manics:

When you run pre-commit can you add a .git-blame-ignore-revs too? It's feature in Git that GitHub now supports, and should mean the repo-wide auto-formatting commits are skipped in the blame view:

https://docs.github.com/en/repositories/working-with-files/using-files/viewing-a-file#ignore-commits-in-the-blame-view

I ran into a failure because the values.yaml duplicated code had been outdated. So instead of re-running it manually I decided to add a pre-commit hook to do it for us which I think should work for pre-commit.ci as well.

choldgraf commented 2 years ago

+1 from me, only comment is that maybe we should use isort instead, since @minrk liked that one for the jupyterhub repo?

choldgraf commented 2 years ago

sure I don't have strong opinions, just feel like if we're going to go with an autoformatter standard, we should at least standardize on the autoformatter we use haha. These PRs tend to be somewhat disruptive since they often invalidate other open PRs, so it might not be trivial to just do a follow-up PR. This is why I thought it might be worth quickly agreeing on something if it was going to be quick. But if you think this will require more conversation then I agree we should just merge this. @consideRatio what do you think?

minrk commented 2 years ago

+1 to merge, with or without isort. There aren't so many files here so the difference should be small.

If you do add isort, you need to add

[tool.isort]
profile = "black"

to pyproject.toml for isort to play nice with black.

minrk commented 2 years ago

Reviewed the blame output with the ignore-refs config. Pretty cool!

consideRatio commented 2 years ago

Ping @manics @minrk @choldgraf sorry for the noise but I'd love to see this merged so I can take this off my radar.

isort is now used instead of reorder-python-imports
I added a pre-commit hook to use the local script that updates a values.yaml file entry with code from binderspawner_mixin.py
I used that pre-commit hook to generate another commit instead of doing it manually
I've updated .git-black-ignore-revs to mention these commits as commits to ignore when considering git blame.

choldgraf commented 2 years ago

Yesssss Erik you're awesome 😎