Format Slice Expressions #5047
Format Slice Expressions #5047
Conversation
|
Current dependencies on/for this PR:
This comment was auto-generated by Graphite. |
konstin
force-pushed
the
format-expr-slice
branch
from
adc3036 to
aaaa79d
Compare
PR Check ResultsEcosystem✅ ecosystem check detected no changes. BenchmarkLinuxWindows |
konstin
force-pushed
the
format-expr-slice
branch
from
aaaa79d to
0402d6e
Compare
| @@ -833,6 +835,87 @@ fn handle_module_level_own_line_comment_before_class_or_function_comment<'a>( | |||
| } | |||
| } | |||
|
|
|||
| /// Handles the attaching the | |||
There was a problem hiding this comment.
This comment seems incomplete
| @@ -439,6 +439,14 @@ impl<'a> DecoratedComment<'a> { | |||
| pub(super) fn text_position(&self) -> CommentTextPosition { | |||
| self.text_position | |||
| } | |||
|
|
|||
| /// Change the position | |||
| pub(super) fn change_position(self, text_position: CommentTextPosition) -> Self { | |||
There was a problem hiding this comment.
Nit: I think taking a &mut self would be fine here. It could simplify some of your call sites .
| if !lower_trailing_comments.is_empty() { | ||
| // make sure the colon is after the comments | ||
| hard_line_break().fmt(f)?; | ||
| } |
There was a problem hiding this comment.
Would the following also work? It would be cheaper because querying comments is somewhat expensive (not very but it requires a hash map lookup).
write!(f, [group(&format_args[lower.format(), soft_line_break(), first_colon_space, text(:)])]?;Or does that not work because we want to keep the : on the same line as lower even if it expands?
There was a problem hiding this comment.
that sounds like a really complex rewrite and i'd like to avoid that 😬
There was a problem hiding this comment.
You could give it a try for a single expression AND/OR try it as a separate PR.
| if !dangling.is_empty() { | ||
| // put the colon and comments on their own lines | ||
| hard_line_break().fmt(f)?; | ||
| } | ||
| dangling_comments(dangling).fmt(f)?; |
There was a problem hiding this comment.
Nit: We shouldn't call into the dangling comments formatting if we know that there are none.
| if !dangling.is_empty() { | |
| // put the colon and comments on their own lines | |
| hard_line_break().fmt(f)?; | |
| } | |
| dangling_comments(dangling).fmt(f)?; | |
| if !dangling.is_empty() { | |
| // put the colon and comments on their own lines | |
| write!(f, [hard_line_break(), dangling_comments(dangling)])?; | |
| } |
konstin
force-pushed
the
format-expr-slice
branch
from
3b225cb to
c10d30c
Compare
| second_colon | ||
| .as_ref() | ||
| .map_or(slice_dangling_comments.len(), |second_colon| { | ||
| slice_dangling_comments[first_colon_partition_index..] |
There was a problem hiding this comment.
Nit: Would it make sense to already split the halves to avoid the manual slicing e.g. on line 75.
let (dangling_first_colon_comments, slice_dangling_comments) = slice_dangling_comments.split_at(first_colon_partition_index);It also removes the need to manually add the first_colon_partition_index offset to the computed second_colon_partition_index
There was a problem hiding this comment.
done, i refactored the whole splitting
| lower.format().fmt(f)?; | ||
| if comments.has_trailing_comments(lower.as_ref()) { | ||
| // make sure the colon is after the comments | ||
| hard_line_break().fmt(f)?; | ||
| } |
There was a problem hiding this comment.
Nit. You could also use a line_suffix_boundary here. That removes the need for checking if there's a comment. A line_suffix_boundary automatically enforces a hard line break if there's any pending line_suffix (trailing comment)
| lower.format().fmt(f)?; | |
| if comments.has_trailing_comments(lower.as_ref()) { | |
| // make sure the colon is after the comments | |
| hard_line_break().fmt(f)?; | |
| } | |
| write!(f, [left.format(), line_suffix_boundary()])?; |
| if !all_simple { | ||
| space().fmt(f)?; | ||
| } | ||
| text(":").fmt(f)?; |
There was a problem hiding this comment.
You don't like the write macro, do you ;)
There was a problem hiding this comment.
procedural code is kinda convenient 🙈
| fn leading_comments_spacing( | ||
| f: &mut PyFormatter, | ||
| leading_comments: &[SourceComment], | ||
| ) -> FormatResult<()> { | ||
| if let Some(first) = leading_comments.first() { | ||
| if first.line_position().is_own_line() { | ||
| // Insert a newline after the colon so the comment ends up on its own line | ||
| hard_line_break().fmt(f)?; | ||
| } else { | ||
| // Insert the two spaces between the colon and the end-of-line comment after the colon | ||
| write!(f, [space(), space()])?; | ||
| } | ||
| } | ||
| Ok(()) |
There was a problem hiding this comment.
Nit: An alternative would be to format all end_of_line comments as trailing_comments which does the whitespace normalization for you but it may be more complicated in the end
| upper.format().fmt(f)?; | ||
| if comments.has_trailing_comments(upper.as_ref()) { | ||
| // make sure the colon is after the comments | ||
| hard_line_break().fmt(f)?; | ||
| } |
There was a problem hiding this comment.
Nit: use a line suffix boundary to avoid checking comments for trailing comments
|
|
||
| let comments = f.context().comments().clone(); | ||
| let dangling_comments = comments.dangling_comments(item.as_any_node_ref()); | ||
| debug_assert!(dangling_comments.len() <= 1); |
There was a problem hiding this comment.
What's the reason for this assertion? What happens if it is violated in production? Is it necessary because we use trailing_comments instead of dangling_comments during formatting?
There was a problem hiding this comment.
The only dangling comment a subscript can have is the one after the [, the rest has been assigned to the slice expression.
What happens if it is violated in production?
Bad, potentially unstable formatting. I can see no case where it would be violated, and there is none in the cpython code base.
konstin
force-pushed
the
format-expr-slice
branch
from
9e568e0 to
ad00427
Compare
This formats slice expressions and subscript expressions.
Spaces around the colons follows the same rules as black (https://black.readthedocs.io/en/stable/the_black_code_style/current_style.html#slices):
Comment placement is different due to our very different infrastructure. If we have explicit bounds (e.g.
x[1:2]) all comments get assigned as leading or trailing to the bound expression. If a bound is missing[:], comments get marked as dangling and placed in the same section as they were originally in:to
Except for the potential trailing end-of-line comments, all comments get formatted on their own line. This can be improved by keeping end-of-line comments after the opening bracket or after a colon as such but the changes were already complex enough.
I added tests for comment placement and spaces.