Skip to content
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 #3932

Merged
merged 5 commits into from
Oct 10, 2023
Merged

Standardise newlines after module-level docstrings #3932

merged 5 commits into from
Oct 10, 2023

Conversation

DanielNoord
Copy link
Contributor

@DanielNoord DanielNoord commented Oct 8, 2023

Description

Resolves #1872.

This is a rebase of #2996 and therefore most credit should go to @jpy-git.

I have excluded the refactoring that was included in the rebase attempt at #3287. I feel the refactoring makes the PR unnecessarily hard to review and rebase. It's probably a very desired refactor (I can't comment on this since I don't know the codebase too well), but it can be taken up in a specific PR.

I won't autoclose those PRs with this PR as I think the refactoring should probably be rebased on its own and not lost to the closed PR list.

I would appreciate a review as I'm not sure this is the right approach for the checks, it seemed to be the best compromise between main and the various versions of the check that @jpy-git wrote in their original PR. Happy to change it to another check if desired. I'd really like to see this change make it into the preview so happy to help wherever needed to get this to pass.

Checklist - did you ...

  • Add an entry in CHANGES.md if necessary?
  • Add / update tests if necessary?
  • Add new / update outdated documentation?

@@ -0,0 +1,25 @@
"""Single line module-level docstring should be followed by single newline."""
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't think we'll run this test as it's not in simple_cases/.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Moved the tests to preview/ to make sure they run under preview mode. They executed now with pytest -k module_docstring.

Also added another two tests cases of which I thought the original PR didn't really have a good test:

  1. A file that already has a new line in-between the docstring and the rest of code and
  2. A file that has no lines in between the docstring and the rest of code and should thus get one

@github-actions
Copy link

github-actions bot commented Oct 8, 2023

diff-shades results comparing this PR (03b7281) to main (5d5bf6e). The full diff is available in the logs under the "Generate HTML diff report" step.

╭──────────────────────── Summary ─────────────────────────╮
│ 15 projects & 626 files changed / 626 changes [+608/-18] │
│                                                          │
│ ... out of 2 495 902 lines, 11 740 files & 23 projects   │
╰──────────────────────────────────────────────────────────╯

Differences found.

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

Copy link
Collaborator

@JelleZijlstra JelleZijlstra left a comment

Choose a reason for hiding this comment

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

Thanks! Pushed a change moving the tests as I just changed the structure of the tests.

@JelleZijlstra JelleZijlstra merged commit b7717c3 into psf:main Oct 10, 2023
34 checks passed
@DanielNoord DanielNoord deleted the module_docstring_newlines_rebase branch November 23, 2023 09:41
@chalecado
Copy link

how can i exclude this from being enforced on my project? its caused 100s of files to need to change

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.

Enforce blank line after module docstring
3 participants