Format function argument separator comments #5211
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This was referenced Jun 20, 2023
|
Current dependencies on/for this PR:
This comment was auto-generated by Graphite. |
PR Check ResultsEcosystem✅ ecosystem check detected no changes. BenchmarkLinuxWindows |
konstin
force-pushed
the
format_function_argument_separator_comments
branch
from
f3f2a6e
to
93b9a3e
Compare
konstin
force-pushed
the
fix_cypthon_errors_set_2
branch
from
a3330e9
to
3e00c0f
Compare
konstin
force-pushed
the
format_function_argument_separator_comments
branch
from
93b9a3e
to
6cc0256
Compare
MichaReiser
approved these changes
Jun 21, 2023
Comment on lines
267
to
270
| let slash = if arguments.posonlyargs.is_empty() { | ||
| // If there are no positional only arguments the default handling below is wrong | ||
| None | ||
| } else if let Some(preceding_end) = arguments.posonlyargs.last().map(Ranged::end) { |
There was a problem hiding this comment.
Can we remove this branch? The else branch returns None and the else if can never be true if posonlyargs is empty.
There was a problem hiding this comment.
good catch, this was remnant of the defaults list time
| }; | ||
|
|
||
| // If we have a vararg we have a node that the comments attach to | ||
| let star = if arguments.kwonlyargs.is_empty() || arguments.vararg.is_some() { |
There was a problem hiding this comment.
Nit: arguments.kewonlyargs.is_empty is already handled by the else branch (the only case where .first() returns None)
| /// ^^^^^^ keyword only arguments (kwargs) | ||
| /// ``` | ||
| pub(crate) fn assign_argument_separator_comment_placement( | ||
| slash: Option<(TextSize, TextRange, TextSize)>, |
There was a problem hiding this comment.
Nit: Define a struct with named fields. It's hard to guess the meaning of the text positions and range.
There was a problem hiding this comment.
refactored the entire code
## Summary This is a complete rewrite of the handling of `/` and `*` comment handling in function signatures. The key problem is that slash and star don't have a note. We now parse out the positions of slash and star and their respective preceding and following note. I've left code comments for each possible case of function signature structure and comment placement ## Test Plan I extended the function statement fixtures with cases that i found. If you have more weird edge cases your input would be appreciated.
konstin
force-pushed
the
format_function_argument_separator_comments
branch
from
fa2db93
to
3862051
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary
This is a complete rewrite of the handling of
/and*comment handling in function signatures. The key problem is that slash and star don't have a note. We now parse out the positions of slash and star and their respective preceding and following note. I've left code comments for each possible case of function signature structure and comment placementTest Plan
I extended the function statement fixtures with cases that i found. If you have more weird edge cases your input would be appreciated.