Skip to content

Comments

Standardise newlines after module-level docstrings#2996

Closed
jpy-git wants to merge 13 commits intopsf:mainfrom
jpy-git:module_docstring_newlines
Closed

Standardise newlines after module-level docstrings#2996
jpy-git wants to merge 13 commits intopsf:mainfrom
jpy-git:module_docstring_newlines

Conversation

@jpy-git
Copy link
Contributor

@jpy-git jpy-git commented Apr 5, 2022 •
edited
Loading

Description

Closes #1872.

Does 2 things:
a) Refactor EmptyLineTracker for more consistent look-back logic. Simplifies logic by removing complex interactions between before and after for various rules.

b) Standardise newlines after module-level docstrings. We now enforce 1 newline after both single and multi line docstrings.
e.g.

"""docstring"""

import foo
"""docstring

blah blah blah
"""

import foo

Checklist - did you ...

  • Add a CHANGELOG entry if necessary?
  • Add / update tests if necessary?
  • Add new / update outdated documentation?

@jpy-git jpy-git force-pushed the module_docstring_newlines branch from 5dc70ed to f64cd77 Compare April 5, 2022 21:42
@github-actions
Copy link

github-actions bot commented Apr 5, 2022 •
edited
Loading

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.

╭──────────────────────── Summary ─────────────────────────╮
│ 14 projects & 572 files changed / 571 changes [+554/-17] │
│                                                          │
│ ... out of 2 206 193 lines, 10 579 files & 23 projects   │
╰──────────────────────────────────────────────────────────╯

Differences found.

What is this? | Workflow run | diff-shades documentation

@jpy-git jpy-git mentioned this pull request Apr 5, 2022
Open
3 tasks
@jpy-git jpy-git force-pushed the module_docstring_newlines branch from a973c88 to 49c16c9 Compare April 5, 2022 23:35
jpy-git
jpy-git commented Apr 6, 2022
View reviewed changes
@JelleZijlstra
Copy link
Collaborator

JelleZijlstra commented Apr 6, 2022

The number of changes seem surprisingly small. Only a few hundred out of 10k files have a multi-line docstring?

@jpy-git
Copy link
Contributor Author

jpy-git commented Apr 6, 2022 •
edited
Loading

No, it makes sense based on the module docstrings that diff-shades checks:

Total counts:
no_docstring:  10151
single_line_docstring:  183
multi_line_docstring:  721

felix-hilden
felix-hilden reviewed Apr 11, 2022
View reviewed changes
Copy link
Collaborator

@felix-hilden felix-hilden left a 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!

@jpy-git
Copy link
Contributor Author

jpy-git commented Apr 12, 2022 •
edited
Loading

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.

Good point, will update to reference the specific preview mode 😄

I think I'd prefer the newlines to be added to multiline docstrings as well, just for consistency.

Sounds good 👍

@jpy-git jpy-git mentioned this pull request Apr 27, 2022
Merged
3 tasks
@jpy-git jpy-git requested a review from felix-hilden April 27, 2022 11:13
felix-hilden
felix-hilden reviewed May 2, 2022
View reviewed changes
Copy link
Collaborator

@felix-hilden felix-hilden left a 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.

@cdce8p cdce8p mentioned this pull request May 12, 2022
Merged
@DanielNoord DanielNoord mentioned this pull request May 14, 2022
Merged
2 tasks
@felix-hilden felix-hilden mentioned this pull request May 20, 2022
Closed
@felix-hilden felix-hilden mentioned this pull request Jun 26, 2022
Closed
@ichard26 ichard26 self-requested a review July 6, 2022 17:45
ichard26
ichard26 reviewed Jul 7, 2022
View reviewed changes
Copy link
Collaborator

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

  1. I love the refactor, it makes EmptyLineTracker so much nicer to follow :) ↩

@ichard26 ichard26 mentioned this pull request Aug 13, 2022
Open
@ichard26 ichard26 added this to the Release 22.9.0 milestone Aug 29, 2022
@ichard26 ichard26 added the F: empty lines Wasting vertical space efficiently. label Aug 29, 2022
@ichard26 ichard26 self-assigned this Aug 29, 2022
@ichard26 ichard26 mentioned this pull request Sep 26, 2022
Closed
3 tasks
@ichard26 ichard26 removed this from the Release 22.9.0 milestone Sep 29, 2022
@JelleZijlstra
Copy link
Collaborator

JelleZijlstra commented Dec 29, 2022

Unfortunately this has a lot of merge conflicts, sorry for dropping the ball here.

@aqeelat
Copy link

aqeelat commented Jun 29, 2023

@JelleZijlstra does it mean that you need help rebasing it?

@JelleZijlstra
Copy link
Collaborator

JelleZijlstra commented Oct 8, 2023

Yes, somebody will have to rebase this.

@DanielNoord
Copy link
Contributor

DanielNoord commented Oct 8, 2023

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

@DanielNoord DanielNoord mentioned this pull request Oct 8, 2023
Merged
3 tasks
@DanielNoord
Copy link
Contributor

DanielNoord commented Oct 8, 2023

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!

@JelleZijlstra
Copy link
Collaborator

JelleZijlstra commented Oct 10, 2023

Merged #3932, thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@felix-hilden felix-hilden felix-hilden left review comments

@ichard26 ichard26 ichard26 left review comments

Assignees

@ichard26 ichard26

Labels

F: empty lines Wasting vertical space efficiently.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Enforce blank line after module docstring

6 participants

@jpy-git @JelleZijlstra @aqeelat @DanielNoord @felix-hilden @ichard26