-
Notifications
You must be signed in to change notification settings - Fork 565
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
Fix black #1763
Fix black #1763
Conversation
requirements.dev.txt
Outdated
coverage | ||
doctest-ignore-unicode==0.1.2 | ||
flake8 | ||
flake8-black |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why remove this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the PR @eggplants it looks good. I am in favor of not pinning the version of black, though I think if possible we should try and figure out why this was done to begin with.
Also flake8-black is quite useful as it integrates with VSCode, I would not remove it unless there is a specific problem it causes.
Co-authored-by: Iwan Aucamp <[email protected]>
As you can see of this: flake8-black 0.3.2 (latest) outputs many unexpected and useless error like this:
This error is caused by shebang lines. Could I remove their shebang to resolve BLK999? |
The problem I reported some time ago seems to be recurring. |
Re: pinning of black version
This was done because black was, a year or two ago, undergoing rapid development and its outputs were different between versions which caused Git diffs as people used different versions over time. So we pinned to one version to lock things into being exactly the same. I think we can pin to black per major or semi-major release of RDFLib so we can update black over time, just not with every PR! |
Thats right. Eg, if I had v21.2b0 installed in my development environment, ran
Now Black is out of the recent January release, |
I agree that we should keep pinning it, but I think every so often we can update what we have pinned to, like every major release. So let's now pin to 22.1.0. |
@nicholascar @eggplants So that means the output of Black will be the same for the whole calendar year. So we can set the requirements-dev.txt line to |
Makes sense that they have such a policy! Thanks for finding it. So let's do this. |
Partial version spec does not work for me: $ .venv/bin/black --version
black, 22.1.0 (compiled: yes)
$ .venv/bin/black rdflib
Oh no! 💥 💔 💥 The required version `22` does not match the running version `22.1.0`!
$ .venv/bin/black --required-version 22 rdflib
Oh no! 💥 💔 💥 The required version `22` does not match the running version `22.1.0`!
$ .venv/bin/black --required-version 22.1.0 rdflib
All done! ✨ 🍰 ✨
116 files left unchanged. Also, if we want to use a fixed version with pre-commit we sadly have to keep the local hook as it was before this PR. I will push changes to your branch @eggplants that uses the latest exact version, I would like it if there was some way to make a partial version spec (i.e. version range) work, but I can't find any. |
Seems at least for me black wants an exact version ```console $ .venv/bin/black --version black, 22.1.0 (compiled: yes) $ .venv/bin/black rdflib Oh no! 💥 💔 💥 The required version `22` does not match the running version `22.1.0`! $ .venv/bin/black --required-version 22 rdflib Oh no! 💥 💔 💥 The required version `22` does not match the running version `22.1.0`! $ .venv/bin/black --required-version 22.1.0 rdflib All done! ✨ 🍰 ✨ 116 files left unchanged. ``` Also revert back to the local hook for pre-commit as this is needed when using a fixed version.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, looks like it might be as good as we can get it and at least we've updated to a major stable black version
@ashleysommer @aucampia @nicholascar Thank you very much for the correction to my poor PR! I hope this makes CI better. But flake8-black's nonsense errors still are output. |
Thanks you @eggplants for kicking off this improvement! Will merge as soon as I see a confirmation of the final PR from @ashleysommer or @aucampia |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me.
Ref: #1762
I changed to install and use black in the environment created by pre-commit instead of the local one when running pre-commit hooks.