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

🔧 MAINTAIN: Move format & lint to pre-commit #161

Merged
merged 3 commits into from
Sep 18, 2021

Conversation

chrisjsewell
Copy link
Contributor

@chrisjsewell chrisjsewell commented Sep 15, 2021

I've removed flake8 and check-manifest from the tox configuration, and moved them to a pre-commit configuration.
There I have also added the https://github.com/pycqa/isort, https://github.com/asottile/pyupgrade, and https://github.com/psf/black hooks

Note, you could also move mypy to pre-commit, but the only draw back is that you have to specify all the dependencies directly in the pre-commit config.
(I also moved the mypy.ini config to pyproject.toml)

I added a pre-commit job in the GH Actions, but alternatively you could also install/activate https://pre-commit.ci/

Finally, I added a little extra in CONTRIBUTING.md, mentioning tox and pre-commit

Copy link
Contributor

@choldgraf choldgraf left a comment

Choose a reason for hiding this comment

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

+1 from me - if I understand correctly this isn't actually changing any practices, just automating them (e.g., we already say black is used by the codebase in our README). The main new thing this adds is pre-commit and this is already used across a bunch of Jupyter repositories.

my only question would be: shouldn't we need to add pre-commit to a requirement file somewhere? I would have assumed we need to add it to https://github.com/jupyter/nbclient/blob/master/requirements-dev.txt

@chrisjsewell
Copy link
Contributor Author

chrisjsewell commented Sep 16, 2021

Yep cheers (FYI this was already loosely agreed #151 (comment)).

if I understand correctly this isn't actually changing any practices, just automating them

not particularly no; although going by the amount of changes I guess the codebase wasn't fully "blackd" yet. I'll leave you guys to decide if you would rather remove any hooks (like pyupgrade).

my only question would be: shouldn't we need to add pre-commit to a requirement file somewhere? I would have assumed we need to add it to master/requirements-dev.txt

Hmm, personally I would say no; the dev-requirements are specific to this project and you would not want to install them all in to your root python environment, whereas pre-commit and tox are more "meta requirements" that you would do this with (well actually I use https://github.com/pypa/pipx) and use for basically every python project.
You, just about, don't have to have a specific version as they are intended to be very stable.
Plus, I've already added the recommendation to use them to CONTRIBUTING.md

@choldgraf
Copy link
Contributor

choldgraf commented Sep 16, 2021

Right - so this PR basically:

  1. Applies conventions that we've already agreed to (like black, mypy, etc)
  2. Automates those conventions via pre-commit
  3. Documents the conventions / automation

IMO in the long term we should use pre-commit.ci since it'll automate even further, but I'm +1 on merging this as-is so that we don't bottleneck iterative progress.

Maybe @davidbrochart wants to give a quick 👍 or request changes since he's the one in that linked comment? But IMO we can leave this open a couple days and if nobody speaks up, merge it in (since this will also become stale pretty quickly)

@chrisjsewell
Copy link
Contributor Author

Indeedio 👍

@davidbrochart
Copy link
Member

That's great @chrisjsewell, thanks a lot!

@choldgraf
Copy link
Contributor

cool - I'll go ahead and merge this one in, since it's going to become out of date very quickly since it's blackifying stuff that should have been blackified before :-)

thanks @chrisjsewell for the contribution 🎉

@choldgraf choldgraf merged commit 2a6b8aa into jupyter:master Sep 18, 2021
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.

3 participants