Conversation
|
Current dependencies on/for this PR:
This comment was auto-generated by Graphite. |
adc3036 to
aaaa79d
Compare
PR Check ResultsEcosystem✅ ecosystem check detected no changes. BenchmarkLinuxWindows |
aaaa79d to
0402d6e
Compare
| } | ||
| } | ||
|
|
||
| /// Handles the attaching the |
There was a problem hiding this comment.
This comment seems incomplete
| } | ||
|
|
||
| /// 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)])?; | |
| } |
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.
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.