8000 style: black the codebase by henryiii · Pull Request #665 · scikit-build/scikit-build · GitHub
[go: up one dir, main page]

Skip to content

style: black the codebase #665

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

Merged
merged 4 commits into from
Feb 18, 2022

Conversation

henryiii
Copy link
Contributor

This adds black formatting. It uses the last beta release of black instead of
Black 22.1.0 since black no longer supports targeting Python 2. We can upgrade
once we drop Python 2 support very soon (pybind11 dropped support yesterday!).

  • chore: adding black
  • style: run black in pre-commit

Copy link
Contributor
@jcfr jcfr 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 adding the style check, automating the process makes sense 👌

Is there way to keep the single quote, while this may not be the current recommendation, it introduced a lot of noise in the history and blame output which is quite useful.

The idea was to use single quote for constants. I think there should some prior style commits discussing this.

Could we look at the ratio double quote vs single quotes ?

I also suggest to increase the line length passed to black to match the current settings. It seems some changes are introduced to accommodate a shorter length settings.

Last, I am wondering if adding an editorconfig file would make sense. Most of editor are able to read it these days. That way we could ensure contributions are consistent and minimize the amount of changes.

docs/conf.py Outdated
@@ -20,7 +20,7 @@
# directory, add these directories to sys.path here. If the directory is
# relative to the documentation root, use os.path.abspath to make it
# absolute, like shown here.
sys.path.insert(0, os.path.abspath('.'))
Copy link
Contributor

Choose a reason for hiding this comment

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

Unless sphinx updated the format of the auto generated conf.py, I would suggest to keep this file as it is. It allows to have sphinx updating it for us and allow to easily compare with what sphinx would generate ...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added it to the ignore. Though I don't think I've ever been able to regenerate this file, it tends to need to get customized.

@@ -19,18 +21,30 @@ repos:
- id: isort

- repo: https://github.com/asottile/pyupgrade
rev: v2.31.0
rev: "v2.31.0"
Copy link
Contributor

Choose a reason for hiding this comment

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

I suggest to keep the version without double quote, that all revision are consistently specified... and less noise in the git blame output

Copy link
Contributor Author
@henryiii henryiii Feb 14, 2022

Choose a reason for hiding this comment

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

There can be a problem if a check is a single number (no v in the front - like, for example, scikit-build's tags ;), I believe, so I was moving to consistently placing the strings around the rev's.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Edit: scikit-build, not scikit-hep. Too many scikits. :)

@henryiii henryiii force-pushed the henryiii/style/black branch from 6e4c686 to b4579aa Compare February 14, 2022 20:58
@henryiii
Copy link
Contributor Author

Is there way to keep the single quote, while this may not be the current recommendation, it introduced a lot of noise in the history and blame output which is quite useful.

You can add the style-only changes to a git blame rev-ignore list (.git-blame-rev-ignore is a standard name, IIRC), and then git blame will ignore the stylistic changes when computing blame's. Unfortunately, it is not picked up by GitHub, so it's only manual.

The idea was to use single quote for constants. I think there should some prior style commits discussing this.

Trying to switch based on something that can't be statically checked will just create a random distribution of single and double quotes (which we have now) - I've seen this tried before, and I don't think it's helpful. I used to like single quotes better, but I'm happy enough to go with blacks double quotes if it's a standard. And black's reasoning is correct, that single quotes are more common in normal text and double quotes (as you can count in the text above, I've use single quotes many times, and no double quotes).

Though it's not supported by black, there is a way to force single quotes instead via pre-commit, so we could go that direction. Not found of having them randomly mix, though.

I also suggest to increase the line length passed to black to match the current settings. It seems some changes are introduced to accommodate a shorter length settings.

Done, they match exactly now. I've had developers companion about 120, so maybe ~100 would be good (and black lets it go over by up to 10%).

Last, I am wondering if adding an editorconfig file would make sense. Most of editor are able to read it these days. That way we could ensure contributions are consistent and minimize the amount of changes.

I'd recommend all contributors use pre-commit, and regardless of if they do, pre-commit.ci will automatically ensure everything is consistent - simple things like black will get corrected automatically in PRs without users having to care at all. Though this is also a good idea, and we can add one.

@henryiii
Copy link
Contributor Author

Oops, that didn't fix the quote changes. Will investigate.

@henryiii
Copy link
Contributor Author

FYI, on the blame: https://twitter.com/llanga/status/1163767833706323970?lang=en

@henryiii
Copy link
Contributor Author
henryiii commented Feb 14, 2022

Oops, that didn't fix the quote changes. Will investigate.

Looks like blacken-docs doesn't check the pyproject.toml setting. I think that's fine, it's one docs file. The other quotes are untouched.

I recommend fully following black, and enforcing string normalization to double quotes. Otherwise, we could force string normalization to single quotes in pre-commit, if you really prefer. Currently, we have a random mix of single/double, that might slightly be trying to follow a convention, but mostly just depends on who wrote it (I slip into black-style double quotes quite often, from habit). Making it all happen in a single commit makes it easier to add it to a git ignore list, but I'm happy to build such a file regardless of how many commits it takes, and I try to always use the convention that style: is an automated commit. (This PR should be rebased and merged, not squashed and merged).

Up to you.

@henryiii
Copy link
Contributor Author

Currently, double quotes outnumber single quotes by about 2:1:

$ git grep -o '"' -- '*.py' | wc -l
    4142
$ git grep -o "'" -- '*.py' | wc -l
    2594

@jcfr
Copy link
Contributor
jcfr commented Feb 15, 2022

Thanks for the detailed analysis 🙏

I recommend fully following black, and enforcing string normalization to double quotes
double quotes outnumber single quotes by about 2:1:

And thanks for looking into this and for taking the time to provide more details.

Standardizing to double quotes makes sense 🚀

@henryiii henryiii force-pushed the henryiii/style/black branch from f9d5275 to 1730d81 Compare February 16, 2022 15:21
@henryiii henryiii merged commit e02c9b3 into scikit-build:master Feb 18, 2022
@henryiii henryiii deleted the henryiii/style/black branch February 18, 2022 04:08
@jcfr
Copy link
Contributor
jcfr commented Mar 23, 2022

You can add the style-only changes to a git blame rev-ignore list (.git-blame-rev-ignore is a standard name, IIRC), and then git blame will ignore the stylistic changes when computing blame's. Unfortunately, it is not picked up by GitHub, so it's only manual.

Just noticed this earlier today:

See https://docs.github.com/en/repositories/working-with-files/using-files/viewing-a-file#ignore-commits-in-the-blame-view


image

@henryiii
Copy link
Contributor Author

Nice, been waiting for that to come out of private beta. I can add an ignore file.

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.

2 participants
0