Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 12 commits into from
Apr 1, 2022

Conversation

consideRatio
Copy link
Member

@consideRatio consideRatio commented Mar 30, 2022

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.

@consideRatio consideRatio force-pushed the pr/pre-commit-run branch 2 times, most recently from 0ae262c to 541c9bb Compare March 30, 2022 11:54
@choldgraf
Copy link
Member

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

Copy link
Member

@manics manics left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think isort can come in a follow-up PR, especially given that this represents the current agreed auto-formatting, and it's taken 6+ months to get here 😃

@choldgraf If you're happy with that plan please hit merge!

@choldgraf
Copy link
Member

choldgraf commented Mar 31, 2022

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
Copy link
Member

minrk commented Apr 1, 2022

+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.

@consideRatio consideRatio changed the title pre-commit: run all pre-commit autoformatting hooks pre-commit: run all pre-commit autoformatting hooks, add one for ci/check-embedded-chart-code.py script as well and run it Apr 1, 2022
@minrk
Copy link
Member

minrk commented Apr 1, 2022

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

@consideRatio
Copy link
Member Author

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.

@minrk minrk merged commit 656204b into jupyterhub:master Apr 1, 2022
consideRatio pushed a commit to jupyterhub/helm-chart that referenced this pull request Apr 1, 2022
jupyterhub/binderhub#1465 Merge pull request #1465 from consideRatio/pr/pre-commit-run
@choldgraf
Copy link
Member

Yesssss Erik you're awesome 😎

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants