Skip to content
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

Bump black version #4162

Merged
merged 3 commits into from
Jun 1, 2021
Merged

Conversation

bjlittle
Copy link
Member

@bjlittle bjlittle commented Jun 1, 2021

🚀 Pull Request

Description

This PR updates the black version to the latest offering by unpinning.

I did consider removing black from the list of dependencies, but we do require to include black for those that want to test iris with nox on their local host.

Rather than continually update the specific black version in several places, I suggest we simply let the black version float to the latest version at environment package dependency resolve time, and allow the pre-commit-bot to notify us when a later version of black is available to the community.

Seems like a reasonable compromise to take here.


Consult Iris pull request check list

Copy link
Contributor

@trexfeathers trexfeathers left a comment

Choose a reason for hiding this comment

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

we do require to include black for those that want to test iris with nox on their local host.

Since Nox takes does its own pip install via Session.install(), I don't think this is true.

I therefore think we can get away with only requiring Black in the pre-commit config and in the noxfile. And we can have the noxfile read the pre-commit file to get a version. See my suggestions.

noxfile.py Outdated Show resolved Hide resolved
requirements/ci/py36.yml Outdated Show resolved Hide resolved
requirements/ci/py37.yml Outdated Show resolved Hide resolved
requirements/ci/py38.yml Outdated Show resolved Hide resolved
requirements/test.txt Outdated Show resolved Hide resolved
@bjlittle
Copy link
Member Author

bjlittle commented Jun 1, 2021

It's not necessarily all about black here... as a developer I might want all the dependencies, but not run the tests. This could also apply to other packages like flake8.

As a minimal change dropping the pin seems appropriate.

If we want to start to rationalise the dependencies that we explicitly include, then I think that's a different PR entirely - just so as to not muddy the waters here.

@bjlittle
Copy link
Member Author

bjlittle commented Jun 1, 2021

I've got thoughts on how we can go about that, and I don't want to make that the focus of this PR. Also, it might be worth a separate dev discussion before committing changes to code.

@trexfeathers
Copy link
Contributor

OK granted, but there is still a blocker to merging this one.

Pre-commit is the gatekeeper for commits, while Nox is the gatekeeper for merging PR's. If anyone pushes a PR before we get round to merging a pre-commit auto-update, we will get a very confusing PR.

So for me the bare minimum is getting Nox and pre-commit working with the same version. If there's something wrong with my suggestion I think we'll need to keep those two manually aligned as they are at the moment.

@bjlittle
Copy link
Member Author

bjlittle commented Jun 1, 2021

Your suggested change only fits the use case for developer using pre-commit, which isn't a given. So they could commit changes with a later version of black that's ahead of pre-commit and nox.

I'm just going to backtrack and pin again. Safest option all round.

@trexfeathers trexfeathers merged commit 31ea17c into SciTools:master Jun 1, 2021
@bjlittle
Copy link
Member Author

bjlittle commented Jun 1, 2021

@trexfeathers Nice one thanks.

Slightly more interesting than I was bargaining for 😉 ... although worth thinking about 🤔

tkknight added a commit to tkknight/iris that referenced this pull request Jun 4, 2021
* master:
  refactor setup.py to setup.cfg (SciTools#4168)
  update docs pypi release (SciTools#4173)
  Update CI environment lockfiles (SciTools#4137)
  update CONTRIBUTING.md (SciTools#4165)
  RTD support link update (SciTools#4166)
  drop py36 support (SciTools#4163)
  github issues contact link for discussions (SciTools#4164)
  Bump black version (SciTools#4162)
  Stop CI from clobbering commits on lockfile updates (SciTools#4157)
  [pre-commit.ci] pre-commit autoupdate (SciTools#4161)
  Add a method to return a CubeList from CubeList.copy() (SciTools#4094)
  Update black et al (SciTools#4155)
@bjlittle bjlittle deleted the bump-black-version branch June 4, 2021 23:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants