Add pre-commit: pretter, black, pyupgrade, and misc

[consideRatio]

UPDATED PR STATUS: Awaiting resolution of #1390

I think there is no easy way of getting this done initially, but I figured I'll make a go for it. WDYT?

I opted to not apply prettier on js/html/css at this point as it became less readable code when only two spaces was enforced. We can run prettier twice with different settings for html/css/js vs markdown/yaml/json etc if we want to handle that in the future - of which I made a comment about.

Related

• A team-compass issue about trialing pre-commit.ci: try pre-commit.ci? team-compass#379
• A team-compass issue about using pyupgrade: Default to adopt pyugprade in our repos team-compass#453

Action points

• Extract manually made changes from this PR (@consideRatio)
  Remove weird executable flags on files #1384
  Remove unused imports #1385
  Fix various flake8 warnings #1386
  Use f-strings where pyupgrade doesn't handle it automatically #1387
  Formatting adjustments to make the code transform nicer with black autoformatting #1388
  Remove whitespace for flake8 #1389
• Make this PR exclude everything but the config changes
• Mitigate issues for existing PRs (everyone)
  Discussion on how in Add pre-commit: pretter, black, pyupgrade, and misc #1381 (comment)
  A dedicated issue to handle the situation: Give attention to and triage in our pull request list #1390
• Merge this PR
• Open a new PR

  1. # create individual commits for each kind of code change
     pre-commit run --all pyupgrade
     git commit -am "pre-commit: run pyupgrade"
     pre-commit run --all black
     git commit -am "pre-commit: run black"
     pre-commit run --all reorder-python-imports
     git commit -am "pre-commit: run reorder-python-imports"
     pre-commit run --all prettier
     git commit -am "pre-commit: run prettier"
     pre-commit run --all end-of-file-fixer
     git commit -am "pre-commit: run end-of-file-fixer"
     pre-commit run --all requirements-txt-fixer
     git commit -am "pre-commit: run requirements-txt-fixer"
     
     # Verify everything now succeds
     pre-commit run --all

  2. Update helm-chart/binderhub/values.yaml with contents of binderspawner-mixin.py
• Merge the new PR
• Enable pre-commit.ci at https://github.com/organizations/jupyterhub/settings/installations/17782968/update

[consideRatio]

Draft reason - I'm debugging a test failure.

Resolved by not making <script ...></script> be <script ... /> as I think prettier made me attempt, which was wrong.

Tag omission: None, both the starting and ending tag are mandatory.
Source: https://developer.mozilla.org/en-US/docs/Web/HTML/Element/script

[yuvipanda]

Also, if we merge this, we'll probably cause immense merge conflicts with most other open PRs...

[manics]

Could we pull out the manual fixes here into their own PR and just merge those?

Hopefully it'll then be easy to re-base this PR and re-run pre-commit with no manual changes when it's convenient?

[consideRatio]

Could we pull out the manual fixes here into their own PR and just merge those?

Hopefully it'll then be easy to re-base this PR and re-run pre-commit with no manual changes when it's convenient?

I'll do that.

Also, if we merge this, we'll probably cause immense merge conflicts with most other open PRs...

Yeah, I see no easy solution or set of action points to avoid this. I guess the only thing to do is to make a thorough work nudging all open PRs to resolving into merge/close.

[betatim]

Can we run the pre-commit checks in GH Actions instead of adding yet another CI provider? The more different providers we use the higher the chances that one of them has an outage, makes changes we have to react to, etc. GH Actions seems to be running well and we use it a lot, so if we can get the tools/checks executed there that would be a win if you ask me.

On the topic of this PR changing a lot of code which makes it hard to review the changes (triggered by questions like "is this change really a good one?" as well as making life harder for existing PRs: what concrete problem or pain does a tool like pyupgrade address?

[manics]

On the topic of this PR changing a lot of code which makes it hard to review the changes (triggered by questions like "is this change really a good one?" as well as making life harder for existing PRs: what concrete problem or pain does a tool like pyupgrade address?

There was a discussion about pyupgrade on jupyterhub/jupyterhub#3583 (that PR was split into two and merged). I think the big advantage of pyupgade, and autoformatters such as Black or Prettier, is it helps standardise our code which has developed over several years by several different people. If you're new to the code base the different coding styles can be very noticeable and confusing, e.g. you're not sure if the different conventions have a functional effect or not.

It also improves general readibility. It's definitely not perfect, but after a lot of prevaricating I now tend to agree with the developers of autoformatters that everyone has there own preference, so in the absence of a strong reason it's better to just go with the defaults.

You've indirectly brought up a good point about how we make these sort of wide-ranging changes though, and is perhaps something we should think about in future. It fits in with the discussion about how we make impactful changes such as switching to JupyterLab, except this is developer focussed instead of user focussed.

[consideRatio]

@betatim regarding pre-commit.ci, I wrote a comment about it in jupyterhub/team-compass#379 (comment) opened to discuss usage of pre-commit.ci.

You've indirectly brought up a good point about how we make these sort of wide-ranging changes though, and is perhaps something we should think about in future. It fits in with the discussion about how we make impactful changes such as switching to JupyterLab, except this is developer focussed instead of user focussed.

Should I perhaps open an issue dedicated to try highlight what it does and with my suggestion to start using it across our repos? I can champion this one because I'm at the stage where I'd like to see it adopted more widely after having seen it piloted successfully without any issues in jupyter/docker-stacks and jupyterhub/jupyterhub now.

UPDATE: I opened jupyterhub/team-compass#453

[manics]

How's this for a roll-out plan for the main autoformatting change:

• Make a list of all open BinderHub PRs that are still relevant, and are close to being ready to merge
• Once we the list decide on a deadline based on the changes required to those PRs, e.g. one or two months?
• Communicate on those PRs and help with getting them merged

[consideRatio]

How's this for a roll-out plan for the main autoformatting change:

I opened up a dedicated issue for this @manics - #1390

[choldgraf]

Thanks @consideRatio for opening up this one - I think that these kinds of mega-PRs are quite tricky because it's so hard to review anything. I've found that the following is a useful order of operations:

1. Get agreement from the team about which conventions to automatically adopt (for this, I suggest we open up a team-compass issue that makes the proposal "Adopt black and pyupgrade as standards across our repositories"). I think it'd be better to just have this discussion at the team level rather than discussing and implementing on a repo-by-repo basis each time
2. The act of "approving" this should be merging this as team practice into our team compass docs.
3. If that is approved, then assume the following step has approval in all of our repositories, and we welcome implementation:
4. Make a PR that only does 2 things: 1. Adds the approved formatters to our precommit hooks, 2. Runs the pre-commit hooks on our codebase.

If there are other changes that need to be made, I think it's best that we do those separately from the "mega commits" so that it's easier to review.

As for implementing this in the binderhub repository, I think @manics has a good implementation proposal above (#1381 (comment))

[consideRatio]

Thanks for input @choldgraf! I've updated this PR with instructions on how to continue based on your suggested approach.

[manics]

It's been nearly 5 months since this PR was opened. It will cause disruption to open PRs, but most of those are over a year old. Since there's never going to be a perfect time I think we should update this PR and merge it.

[consideRatio]

It's been nearly 5 months since this PR was opened. It will cause disruption to open PRs, but most of those are over a year old. Since there's never going to be a perfect time I think we should update this PR and merge it.

I'm in full agreement of this, let me know if you approve the idea of merging this PR, then I'll address the feedback from Min and rebase this etc for a merge!

[manics]

This was discussed in today's JupyterHub team meeting. We agreed to go ahead with this, and merge in two weeks time (2022-03-03)!

[consideRatio]

@manics rebased and addressed @minrk's wishes about the flake8 config by ignoring all C, E, W kind of warnings!

[manics]

LGTM!

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

[choldgraf]

Woohoo for closing 6 months old PRs!