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

split long function parameter type hints without parentheses #3930

Closed
wants to merge 1 commit into from

Conversation

jakkdl
Copy link
Contributor

@jakkdl jakkdl commented Oct 7, 2023

Description

Resolves #2316 according to the nextline strategy. The poll is currently 10 to 6 of favor of this style, which certainly isn't a slam-dunk, but enough for me to implement it and we can progress the discussion after seeing impacts on existing code.

This is a pretty messy implementation, as can be seen by the sheer length and the complexity check having made me reformat my code several times to break out functions.
I don't love the existence of _split_first_typehint and feel like that should be handled on a second pass by delimiter_split or something. And likewise I ideally wouldn't need any changes in visit_tname after #3899 - I don't really remember why I gave up on resolving it while keeping invisible lpars.

I started out with append_to_line being a subfunction like in delimiter_split and standalone_comment_split, but when the complexity check for good reason wanted me to break up _split_tname I ended up having to add some helper functions to Line and BracketTracker, and while this is not super pretty it's probably better than having append_to_line repeated three times over in the code - and the other uses of it should probably be changed to use _append_to_line as well.

I stumbled upon #3834 just as I thought myself done, and realized that case should clearly also be handled without parentheses. I'm not entirely sure why it isn't, but will probably add that to this PR. derp, it can't use the nextline strategy as per normal type hints since they're not inside parentheses and not tnames. This differentiation being one of the downsides of special-casing parameter lists.

Checklist - did you ...

  • Add an entry in CHANGES.md if necessary?
  • Add / update tests if necessary?
  • Add new / update outdated documentation? (this is maybe noteworthy enough to be documented?)

@github-actions
Copy link

github-actions bot commented Oct 7, 2023

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

╭─────────────────────── Summary ────────────────────────╮
│ 2 projects & 15 files changed / 169 changes [+87/-82]  │
│                                                        │
│ ... out of 2 495 578 lines, 11 739 files & 23 projects │
╰────────────────────────────────────────────────────────╯

Differences found.

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

@jakkdl
Copy link
Contributor Author

jakkdl commented Oct 7, 2023

Hmm, looking at the preview diffs I think I might always want a newline after the colon. So we get

     extLst: Typed[ExtensionList, Literal[True]]
     __elements__: ClassVar[tuple[str, ...]]
     def __init__(
         self,
         custUnit: _HasTagAndGet[_ConvertibleToFloat | None] | _ConvertibleToFloat | None = None,
         builtInUnit:
             _HasTagAndGet[_DisplayUnitsLabelListBuiltInUnit]
             | _DisplayUnitsLabelListBuiltInUnit
             | Literal["none"]
             | None
             = None,
         dispUnitsLbl: DisplayUnitsLabel | None = None,
         extLst: Unused = None,
     ) -> None: ...
 
 class NumericAxis(_BaseAxis):

instead of https://github.com/psf/black/actions/runs/6442045741/job/17492459124?pr=3930#step:10:570

though I'm also personally starting to lean towards this maybe not being much of an improvement, if at all.

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, but several of the test cases look quite bad to me. I'd prefer option (1) in Sebastian's issue: parenthesize it.

None, title="Some long title", description="Some long description"
)
q: str
| None
Copy link
Collaborator

Choose a reason for hiding this comment

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

The old style was much better here.

),
max_jobs: int
| None
= Option(
Copy link
Collaborator

Choose a reason for hiding this comment

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

And here

| loooooooooooooooooooooooooooong
| looooooooooong
| looooooooooong
= 3,
Copy link
Collaborator

Choose a reason for hiding this comment

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

All these examples with defaults look bad. It's as if we're doing long division.

@Zac-HD
Copy link
Contributor

Zac-HD commented Oct 7, 2023

Having now looked at the diffs, I've changed my reaction back on the issue to prefer our current-preview parenthesized style 😓

While I really appreciate @jakkdl's work on this issue, perhaps we should close the issue as solved by #3899 and call it a day?

@jakkdl
Copy link
Contributor Author

jakkdl commented Oct 9, 2023

Sounds like we're all in agreement - a shame to toss out the work I did on this, but I'm quite comfortable in the black codebase now at least! Closing the PR~

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.

PEP 604 and line breaks
3 participants