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

Docstring too long (issue #1632 fix) #3044

Merged
merged 19 commits into from May 8, 2022

Conversation

idorrington92
Copy link
Contributor

@idorrington92 idorrington92 commented May 3, 2022

Description

Fixing this issue #1632 where some docstrings were reformatted to be too long

Checklist - did you ...

  • Add a CHANGELOG entry if necessary? Done
  • Add / update tests if necessary?
  • Add new / update outdated documentation? Don't think this is necessary

@idorrington92 idorrington92 changed the title Docstring too ling Docstring too long (issue #1632 fix) May 3, 2022
@github-actions
Copy link

github-actions bot commented May 3, 2022

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

╭──────────────────────── Summary ────────────────────────╮
│ 5 projects & 61 files changed / 348 changes [+232/-116] │
│                                                         │
│ ... out of 2 204 830 lines, 10 570 files & 23 projects  │
╰─────────────────────────────────────────────────────────╯

Differences found.

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

@felix-hilden
Copy link
Collaborator

felix-hilden commented May 4, 2022

Thanks for submitting! Some tests to validate this behaviour would be in order, and we should add a changelog entry and point to this PR.

@JelleZijlstra should we allow even these smallest and clearly unintended bugs to be fixed in the default style, or should we let this marinate in --preview?

If preview, then we have to add a flag for it in black.mode.Preview and use it to enable this (example PR #2770). I'd suggest a flag called bugfixes or something similar, under which we'll gather all the small things we definitely want to add to the default style.

@JelleZijlstra
Copy link
Collaborator

JelleZijlstra commented May 4, 2022

Yes, this should be in the preview style.

CHANGES.md Outdated Show resolved Hide resolved
@idorrington92 idorrington92 requested a review from JelleZijlstra May 4, 2022
@felix-hilden
Copy link
Collaborator

felix-hilden commented May 4, 2022

In addition to having the flag, we have to use it to enable the change. This would currently change the default behaviour. So something like if Preview.doc_line_length in mode: bugfix. Also my bad for not noticing the change log, I was probably just looking at the first commit

@idorrington92
Copy link
Contributor Author

idorrington92 commented May 4, 2022

In addition to having the flag, we have to use it to enable the change. This would currently change the default behaviour. So something like if Preview.doc_line_length in mode: bugfix. Also my bad for not noticing the change log, I was probably just looking at the first commit

Ah, I think I understand now. I didn't really understand what you meant about the preview but I think I've got it now

src/black/linegen.py Outdated Show resolved Hide resolved
src/black/linegen.py Show resolved Hide resolved
Copy link
Collaborator

@felix-hilden felix-hilden left a comment

Still some comments, hang in there! And thanks for your patience 😄

tests/test_format.py Outdated Show resolved Hide resolved
src/black/linegen.py Outdated Show resolved Hide resolved
idorrington92 and others added 8 commits May 4, 2022
Co-authored-by: Felix Hildén <felix.hilden@gmail.com>
…ultiline strings have the indent included in all lines apart from the first one. Also, if the docstring has more than one line then we need to account for that there is only one set of quotes on the last line, not two
Copy link
Collaborator

@felix-hilden felix-hilden left a comment

The comments are a nice touch 👌 I think I've convinced myself that this is working now, so LGTM! 😄

@JelleZijlstra JelleZijlstra merged commit 20d8ccb into psf:main May 8, 2022
40 checks passed
@ichard26
Copy link
Collaborator

ichard26 commented May 8, 2022

Congrats on your first merged PR to black @idorrington92! 🎉

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.

None yet

4 participants