internal: add merge imports indent tests - #21319
Conversation
rustbot
added
the
S-waiting-on-review
rustbot
removed
the
S-waiting-on-review
0d1109a to
7405f16
Compare
rustbot
added
the
S-waiting-on-review
| let make = SyntaxFactory::without_mappings(); | ||
|
|
||
| let next_ws = self | ||
| let prev_ws = self |
There was a problem hiding this comment.
Isn't this going to leave extra spaces in cases like this?
use foo; // to be removed
// other codeThere was a problem hiding this comment.
Keeping whitespace in this case is also a reasonable solution. Do we need a fallback?
Usually, removing the whitespace before rather than after is more conducive to indentation
There was a problem hiding this comment.
But uses specifically are commonly the first child and followed by a blank line. I don't see a reason to degrade one scenario for the other. If you handle both then fine.
There was a problem hiding this comment.
But
uses specifically are commonly the first child and followed by a blank line.
Uh, no, there are curly and whitespaces before it in ItemList, and there are often whitespaces before it in SourceFile as well
And the test utils will also add a whitespaces line at the beginning
If you handle both then fine.
If the siblings before and after are deleted, it may easily cause overlapping deletion when the caller batch deletes
There was a problem hiding this comment.
A simple heuristic is: remove the previous, and if the use is the first child and has blank lines after it remove them as well.
There was a problem hiding this comment.
In SourceFile, it is rare to truly remove all uses, so it is difficult to leave useless blank lines at the beginning of file
rustfmt will fix the indentation anyway for both ways. If you want to not need that, I won't stop you, but I don't want to regress other cases.
It's possible to make overlapping edits, isn't it?
Nope. It's a bug, I think it will even panic.
There was a problem hiding this comment.
Nope. It's a bug, I think it will even panic.
If it is possible to delete both the before and after uses when deleting a use, then this API will be difficult to use because many uses involve batch deletion of adjacent uses
I won't stop you, but I don't want to regress other cases.
Other cases are not common, and overall, this PR has improved trivias
There was a problem hiding this comment.
If it is possible to delete both the before and after uses when deleting a use, then this API will be difficult to use because many uses involve batch deletion of adjacent uses
What API? Only this assist need to delete things correctly.
Other cases are not common, and overall, this PR has improved trivias
I've stated my opinion. @Veykril what is your opinion?
There was a problem hiding this comment.
Imo we shouldn't spend time on trying to fix whitespace at all as we cannot possibly get it right with the current infra without going through massive hoops. This all will be fixed by us having a proper formatter at some point.
People that do not use rustfmt simply have bad a time with r-a right now.
That is, we shouldn't try to fix indents, but newlines is a different matter though, as rustfmt does not necessarily normalize these. So we should check what happens with that here and if we regress in that regard or not, if yes we shouldn't do this.
There was a problem hiding this comment.
At least for this assist, it replacing the first uses and deleting follows uses, there will be no blank line left on begin of file
I searched all the references, and only this one assist used this call, so there was no any regress
| item | ||
| } | ||
| }; | ||
|
|
There was a problem hiding this comment.
What does this code does? Please leave a comment.
7405f16 to
86e1c9d
Compare
This comment has been minimized.
This comment has been minimized.
|
Given @Veykril agrees with me this is not needed, I'll close this PR. |
rustbot
removed
the
S-waiting-on-review
|
But I noticed that there was no regress:
|
rustbot
added
the
S-waiting-on-review
This comment has been minimized.
This comment has been minimized.
86e1c9d to
8cf59f5
Compare
This comment has been minimized.
This comment has been minimized.
|
@Veykril this is awaiting your decision. |
This comment has been minimized.
This comment has been minimized.
8cf59f5 to
b2df7c1
Compare
|
This PR was rebased onto a different master commit. Here's a range-diff highlighting what actually changed. Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers. |
A4-Tacks
changed the title
|
It seems to have been fixed in #22351, I only retain tests |
rustbot
removed
the
S-waiting-on-review
4 participants
No description provided.