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

Do not put the closing quotes in a docstring on a separate line #3430

Merged
merged 1 commit into from
Dec 13, 2022

Conversation

JelleZijlstra
Copy link
Collaborator

Fixes #3320. Followup from #3044.

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.

Code looks fine in general, but this code is hard to grok so let's see what diff-shades has to say first.

@github-actions
Copy link

diff-shades results comparing this PR (dcdbe2b) to main (80de237). The full diff is available in the logs under the "Generate HTML diff report" step.

╭──────────────────────── Summary ────────────────────────╮
│ 4 projects & 61 files changed / 348 changes [+116/-232] │
│                                                         │
│ ... out of 2 353 640 lines, 10 986 files & 23 projects  │
╰─────────────────────────────────────────────────────────╯

Differences found.

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

@JelleZijlstra
Copy link
Collaborator Author

The diff-shades report is as expected, just removing newlines between one-line docstrings and the closing quotes. Example:

 def _static_hasattr(value: object, attr: str) -> bool:
-    """Returns whether this value has the given attribute, ignoring __getattr__ overrides.
-    """
+    """Returns whether this value has the given attribute, ignoring __getattr__ overrides."""
     try:

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.

On second look, this code looks suspicious.

src/black/linegen.py Show resolved Hide resolved
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.

Actually this seems fine on a third look.

@ichard26 ichard26 merged commit 7d062ec into psf:main Dec 13, 2022
@ichard26
Copy link
Collaborator

Thanks!

@jensguballa
Copy link

I have created a comment on #3044, but as this here is the issue that has been merged, I am providing my thought here again.

After going over the issue I am a little bit puzzled now, as I am not really sure what the intended behavior should be.

My assumption was that black should not apply reformattings which will introduce new line length violations. In my opinion this sounds reasonable, given that you might have a processing pipeline where e.g. flake8 is doing its job hereafter and would fail (like in my case). But it looks like the resolution for this issue is going in another direction.

My view: For the example I have provided black needs to take a decision: either reformatting to a one-line string and violating the maximum line length, or, leaving the doc string as it is and keeping the conformity.

My preference is clearly to keep the conformity, but if I understand the resolution here correctly, reformatting to a one-line string is given priority. I am not in favor of this as special crafting of the doc string by the developer just to work around this issue would go against the philosophy of black.

So would it be reasonable not to reformat the doc string for the example I provided?

@ichard26
Copy link
Collaborator

ichard26 commented Jan 2, 2023

The crux of the problem here is that Black shouldn't move quotes that already violate the line length limit onto a new line (it's ugly and isn't compliant with PEP 257), but also shouldn't move quotes that were already on their own line. This is impossible to do w/ Black's philosophy of avoiding looking at the original formatting as much as possible.

I agree Black shouldn't cause more violations of the line length limit. Technically this introduces one more situation where Black is dependant on pre-existing formatting, but I don't think docstrings can be formatted in an "one size for all" way IMHO.

@jensguballa
Copy link

I do know nothing about black's internals, but for the example I provided would it be possible to implement something like this for one-line doc strings:

Original code (maximum line length not exceeded):

class _YamlBlockStyle(str):
    """Class used to indicate that a string shall be serialized using the block style.
    """

What black produces currently (one-line doc string, maximum line length exceeded):

class _YamlBlockStyle(str):
    """Class used to indicate that a string shall be serialized using the block style."""

What I propose black could produce (multi-line doc string, maximum line length not exceeded):

class _YamlBlockStyle(str):
    """
    Class used to indicate that a string shall be serialized using the block style.
    """

I think that output would comply to the maximum line length as well as to PEP 257. If the latter output would still exceed the maximum line length, then of course there is nothing black can do.

BobDotCom added a commit to Pycord-Development/pycord that referenced this pull request Feb 8, 2023
The preview feature was moved to stable in v23, and the bug previously preventing us from using the new style was fixed in psf/black#3430

Signed-off-by: BobDotCom <71356958+BobDotCom@users.noreply.github.com>
BobDotCom added a commit to Pycord-Development/pycord that referenced this pull request Feb 10, 2023
* chore(pre-commit): pre-commit autoupdate

updates:
- [github.com/PyCQA/autoflake: v2.0.0 → v2.0.1](PyCQA/autoflake@v2.0.0...v2.0.1)
- [github.com/psf/black: 22.12.0 → 23.1.0](psf/black@22.12.0...23.1.0)

* [pre-commit.ci] auto fixes from pre-commit.com hooks

for more information, see https://pre-commit.ci

* remove stale comment

The preview feature was moved to stable in v23, and the bug previously preventing us from using the new style was fixed in psf/black#3430

Signed-off-by: BobDotCom <71356958+BobDotCom@users.noreply.github.com>

* try preview flag for black

Signed-off-by: BobDotCom <71356958+BobDotCom@users.noreply.github.com>

* [pre-commit.ci] auto fixes from pre-commit.com hooks

for more information, see https://pre-commit.ci

* style(pre-commit): disable black --preview flag

---------

Signed-off-by: BobDotCom <71356958+BobDotCom@users.noreply.github.com>
Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
Co-authored-by: BobDotCom <71356958+BobDotCom@users.noreply.github.com>
@adamjstewart
Copy link

If I'm understanding correctly, this PR re-introduces the following issues: #1632, #2274, #3331?

It's great that we're now PEP 257 compliant, but now black reformats my code such that flake8 complains about line length. What's the suggested approach for users using both black and flake8, to add # noqa: E501 to every docstring in this situation? I would argue that this is uglier than having the closing triple quotes on a separate line...

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.

style regression: too long single line docstrings have their quotes moved to a new line.
4 participants