-
Notifications
You must be signed in to change notification settings - Fork 2.5k
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
Standardise newlines after module-level docstrings #2996
Conversation
5dc70ed
to
f64cd77
Compare
diff-shades results comparing this PR (c8275d0) to main (c940f75). The full diff is available in the logs under the "Generate HTML diff report" step.
|
a973c88
to
49c16c9
Compare
The number of changes seem surprisingly small. Only a few hundred out of 10k files have a multi-line docstring? |
No, it makes sense based on the module docstrings that diff-shades checks:
|
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.
Man, huge thanks for taking on all these issues!
I think I'd prefer the newlines to be added to multiline docstrings as well, just for consistency. Not strongly though, and against my own habits and what I said in the original issue I know, but still.
About the preview handling: certainly this is a feature we'd carry over without question, so no biggie, but having the clarity of being able to search for usages of a potential Preview.docstring_newlines
(or similar) was intended to help in picking individual changes for stable. Not that it's likely that we have other changes in EmptyLineTracker
so it would be fairly easy to find this one thing, but anyways. Example PR: #2916
I'm trying to slowly get into the processing code. Your modifications seem sensible, but I'm not the authority on that I'm afraid 😄 I do trust the tests though, nice work!
Good point, will update to reference the specific preview mode 😄
Sounds good 👍 |
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.
Much better, and a nice simplification! Again, provided that other maintainers also agree with the style, and that the implementation itself is sound.
Co-authored-by: Felix Hildén <[email protected]>
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.
Implementation-wise this is good to go1 (minus all 'em merge conflicts >.<), but I need to catch up all of the discussion around docstrings and newlines (and probably quote placement too) before approving the PR as a whole.
Also, once we settle on a style, could we put it in the future code style documentation? I know this is minor, but as we're trying to keep Black relatively stable, we should be explicit when we do change the style.
Footnotes
-
I love the refactor, it makes EmptyLineTracker so much nicer to follow :) ↩
Unfortunately this has a lot of merge conflicts, sorry for dropping the ball here. |
@JelleZijlstra does it mean that you need help rebasing it? |
Yes, somebody will have to rebase this. |
@JelleZijlstra I can't rebase on this PR but I'd be happy to invest the time to get this finished. Are you okay with me rebasing this and opening a new PR? (Or somebody else for that matter) |
Went ahead and created #3932, please consider reviewing it. Very much open to feedback on how to change the code to make the PR mergeable! |
Merged #3932, thanks! |
Description
Closes #1872.
Does 2 things:
a) Refactor
EmptyLineTracker
for more consistent look-back logic. Simplifies logic by removing complex interactions betweenbefore
andafter
for various rules.b) Standardise newlines after module-level docstrings. We now enforce 1 newline after both single and multi line docstrings.
e.g.
Checklist - did you ...