-
-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
Apply black styling to all but vendored modules. #3963
Conversation
Oh, FFS. Ruff is now failing in
I guess ruff counts the carriage return and black doesn't? |
I think it's just a coincidence that the line length is 89. I've tried longer lines and black will create longer lines as long as it's a string that's exceeding the line length, so manual intervention is needed to prevent black from exceeding the line length limit. For example, black will also reformat this to be too long: class C:
dict(
file_defs={
"foobar":
"The quick brown fox jumped over the lazy dog........................",
}
) |
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.
Thank you very much for working on this Jason.
I suppose that it is better to rip off the bandaid and make the code base consistent with the code formatting once and for all.
Am I correct in assuming that the test suite will also make sure that the formatting is consistent in future changes?
@@ -5,11 +5,9 @@ backend-path = ["."] | |||
|
|||
[tool.black] | |||
skip-string-normalization = true | |||
extend_exclude = "_vendor" |
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.
Should we add all the 3rd party and autogenerated code here as well? (i.e. the folders: setuptool/_distutils
and setuptools/config/_validate_pyproject
?)
I have a doubt, will ruff
also follow this config?
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.
Maybe? My instinct is not to add it until needed. I'm pretty sure _distutils
follows black now... and since _validate_pyproject
isn't complaining, let's let it get the black treatment until it becomes untenable to do so (upstream introduces incompatible syntax).
I would not expect ruff
to follow this config, but it seems to be satisfied by the default config, so I'm inclined to stick with it.
"__bootstrap__()", | ||
"" # terminal \n | ||
]) | ||
'\n'.join( |
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.
(Brainstorming)
In a future change, we probably want to rewrite this using f-strings to make it more readable, there are some line breaks that don't make much sense. It would also be nice if we can replace pkg_resources
with improtlib.resources
.
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.
I do agree with using f-strings. I've observed that pyupgrade --py38-plus
automatically converts most formatting strings to f-strings, so maybe we should re-consider running that across the codebase.
Yes. |
I should also point out that adding this change and check makes it easier for me to develop using an IDE that automatically applies black styling on save (avoids the need to format with black and commit before applying a meaningful change). |
Summary of changes
Includes enforcement of the style via pytest-black.
Note the config changes in
pyproject.toml
.Pull Request Checklist
newsfragments/
.(See documentation for details)