Assignment indentation after one line comment #4550
Conversation
|
Thanks for the PR! I gather that this achieves the goal of resolving the referenced issues, but it's another case where we're just trying to triage symptoms instead of the root cause of the core lhs/rhs utility functions not handling comments well. A fairly short time ago, we introduced a new utility function The usage of that function was intentionally kept fairly minimal when it was added to keep that PR reasonable, but I think this is a great opportunity to use that "new" function here to address the root cause instead of continuing to battle side effects. It's a preferable solution both from a code base perspective (encapsulation, reduced duplication) and from a runtime/execution perspective as it avoids first making formatting changes and then having to scan back through our own formatted result to try to figure out what to do next. Would you be willing to try to implement this approach? I believe it should only require making a few tweaks to the rewrite function for locals to use |
|
Made the change for using Had to create a span variable with initial value |
src/formatting/expr.rs
Outdated
| // If last lhs line is only indetation then no need to add a space before rhs | ||
| let result_prefix = if trimmed_last_line_width(lhs) == 0 { | ||
| "" | ||
| } else { | ||
| " " | ||
| }; | ||
| Some(format!("{}{}", result_prefix, new_str)) |
There was a problem hiding this comment.
These changes are unneeded with the change to use rewrite_assign_rhs_with_comments, so should be reverted (this is the type of post processing we should strive to avoid)
There was a problem hiding this comment.
Submitted. Reverted the changes, as all tests pass. However, note that rewrite_pat_expr() is still using choose_rhs() directly and performs format!("{}{}", lhs, rhs), so in principle the removed change could still be relevant until rewrite_pat_expr() will use rewrite_assign_rhs_with_comments()
There was a problem hiding this comment.
so in principle the removed change could still be relevant until rewrite_pat_expr() will use rewrite_assign_rhs_with_comments()
Thanks. We wouldn't want to introduce something like this to the codebase where we already know it's tech debt that would need to be removed. It also wouldn't just apply to pattern expressions, but every single invocation of these functions including the others where it'd be exceptionally duplicative and unnecessary
|
Thanks for the updates! |
* Fix rust-lang#4502 issue - assignment indentation after one line comment * Enhancement by using rewrite_assign_rhs_with_comments() * Changes per comments received
ytmimi
added
1x-backport:pending
* Fix #4502 issue - assignment indentation after one line comment * Enhancement by using rewrite_assign_rhs_with_comments() * Changes per comments received
Changes to fix #4502 issue - proper indentation when there is one-line comment after the
=.Actually #4502 is about two issues - indentation after one-line comment and extra space in the following line. These two issues seemed to be related but it turned out that they are not. I still submitted the fix for both in this PR, as each is relatively simple.
Note that although the expected output of #3851 test cases are modified, as discussed in #4502, no config option was added, assuming that the the previous indentation was a bug and not a feature.