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

fix indentation of line breaks in long type hints by adding parens #3899

Merged
merged 2 commits into from
Sep 22, 2023

Conversation

jakkdl
Copy link
Contributor

@jakkdl jakkdl commented Sep 20, 2023

Description

The simple way of resolving #2316
I would personally prefer to resolve it by formatting it without added parentheses (i.e. option 3 in #2316) to save two unnecessary lines, but that will likely require a bunch of messing around in _maybe_split_omitting_optional_parens and could also impact some other cases - so I think I'll hold off on that one for the moment.

I had to extend maybe_make_parens_invisible_in_atom to also allow expressions, I was quite afraid this would have side effects elsewhere in the code - but it seems to be fine? I could be defensive and add a parameter that enables checking syms.expr, and only set it when called from visit_tname.

There's minor changes to two existing test cases, but they both seem fine to me.

Checklist - did you ...

  • Add an entry in CHANGES.md if necessary? [TODO]
  • Add / update tests if necessary?
  • Add new / update outdated documentation? [I don't think this needs to be done]

@JelleZijlstra
Copy link
Collaborator

Thanks. This looks generally good, but we'll want to make the change only in the preview style for now. Look for existing code like if Preview.parenthesize_conditional_expressions in self.mode: for examples.

@github-actions
Copy link

github-actions bot commented Sep 20, 2023

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

╭──────────────────────── Summary ────────────────────────╮
│ 3 projects & 15 files changed / 279 changes [+138/-141] │
│                                                         │
│ ... out of 2 489 089 lines, 11 731 files & 23 projects  │
╰─────────────────────────────────────────────────────────╯

Differences found.

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

@jakkdl
Copy link
Contributor Author

jakkdl commented Sep 22, 2023

Thanks. This looks generally good, but we'll want to make the change only in the preview style for now. Look for existing code like if Preview.parenthesize_conditional_expressions in self.mode: for examples.

Thanks, fixed!

@JelleZijlstra
Copy link
Collaborator

Looking at the diff-shades output, all changes seem like improvements: https://github.com/psf/black/actions/runs/6275527706/job/17044773640#step:10:1

@JelleZijlstra JelleZijlstra merged commit 8c5d96f into psf:main Sep 22, 2023
34 checks passed
@jakkdl jakkdl deleted the pep604_linebreaks branch September 22, 2023 15:56
@Zac-HD Zac-HD mentioned this pull request Sep 25, 2023
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.

2 participants