style: black the codebase #665
Conversation
There was a problem hiding this comment.
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('.')) | |||
There was a problem hiding this comment.
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 ...
There was a problem hiding this comment.
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" | |||
There was a problem hiding this comment.
I suggest to keep the version without double quote, that all revision are consistently specified... and less noise in the git blame output
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Edit: scikit-build, not scikit-hep. Too many scikits. :)
6e4c686 to
b4579aa
Compare
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.
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.
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%).
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. |
|
Oops, that didn't fix the quote changes. Will investigate. |
|
FYI, on the blame: https://twitter.com/llanga/status/1163767833706323970?lang=en |
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 Up to you. |
|
Currently, double quotes outnumber single quotes by about 2:1: $ git grep -o '"' -- '*.py' | wc -l
4142
$ git grep -o "'" -- '*.py' | wc -l
2594 |
|
Thanks for the detailed analysis 🙏
And thanks for looking into this and for taking the time to provide more details. Standardizing to double quotes makes sense 🚀 |
f9d5275 to
1730d81
Compare
Just noticed this earlier today:
|
|
Nice, been waiting for that to come out of private beta. I can add an ignore file. |
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!).