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

Fix black #1763

Merged
merged 7 commits into from Mar 17, 2022
Merged

Fix black #1763

merged 7 commits into from Mar 17, 2022

Conversation

eggplants
Copy link
Contributor

@eggplants eggplants commented Mar 15, 2022

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.

@eggplants eggplants mentioned this pull request Mar 15, 2022
@eggplants eggplants marked this pull request as ready for review March 15, 2022 20:46
coverage
doctest-ignore-unicode==0.1.2
flake8
flake8-black
Copy link
Member

@aucampia aucampia Mar 15, 2022

Choose a reason for hiding this comment

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

Why remove this?

Copy link
Member

@aucampia aucampia left a 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 <aucampia@gmail.com>
@eggplants
Copy link
Contributor Author

eggplants commented Mar 15, 2022

Why remove this?

As you can see of this:
https://github.com/RDFLib/rdflib/runs/5561399664?check_suite_focus=true

flake8-black 0.3.2 (latest) outputs many unexpected and useless error like this:

rdflib/compare.py:0:1: BLK999 Unexpected exception: int object expected; got str

This error is caused by shebang lines. Could I remove their shebang to resolve BLK999?

@eggplants
Copy link
Contributor Author

The problem I reported some time ago seems to be recurring.
peterjc/flake8-black#47

@nicholascar
Copy link
Member

Re: pinning of black version

I think if possible we should try and figure out why this was done to begin with.

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!

@ashleysommer
Copy link
Contributor

ashleysommer commented Mar 16, 2022

its outputs were different between versions which caused Git diffs as people used different versions over time

Thats right. Eg, if I had v21.2b0 installed in my development environment, ran black before my PR, my PR got merged. But you have black v21.4b0 on your system, now when you run black some of my formatted code gets changed again, causing diff noise in almost every pull request.
Then when we introduced black checking on the CI test pipeline, it used v21.6b0, and now PRs are flagged as non-conformant to Black, even if you formatted it with your version before pushing.
So we decided to settle on "this is the one black version we use for RDFLib", we put that in the documentation, and pinned that in the requirements.
In v21.6b0 black even introduced the --required-version option when running black, so that your check would fail if you were not using the exact given version of the tool, that is to prevent the issues we've seen. We use that in our CI test.

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!

Now Black is out of the recent January release, black v22.1.0 is finally out of Beta. So that means we should definitely update to using that as "the black version we use for RDFLib". However I'm not sure about unpinning it, and I still think we should have --required-version set to match, eg 22.1.0.

@nicholascar
Copy link
Member

I'm not sure about unpinning it

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.

@ashleysommer
Copy link
Contributor

@nicholascar @eggplants
I may have to adjust my recommendation. I just saw the new Black Stability Policy: https://black.readthedocs.io/en/latest/the_black_code_style/index.html#stability-policy

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 >=22.0,<23 and the --required-version to "22", and all will be fine (famous last words).

@nicholascar
Copy link
Member

I may have to adjust my recommendation...

Makes sense that they have such a policy! Thanks for finding it. So let's do this.

@aucampia
Copy link
Member

aucampia commented Mar 16, 2022

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

@nicholascar nicholascar left a 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

@eggplants
Copy link
Contributor Author

eggplants commented Mar 16, 2022

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

@nicholascar
Copy link
Member

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

docs/developers.rst Outdated Show resolved Hide resolved
Copy link
Member

@white-gecko white-gecko left a 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.

@nicholascar nicholascar merged commit 8ef6ece into RDFLib:master Mar 17, 2022
@eggplants eggplants deleted the fix_black branch March 17, 2022 11:11
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.

None yet

5 participants