Remove trailing commas during conversion to class#45778
Remove trailing commas during conversion to class#45778CyrusNajmabadi merged 4 commits intodotnet:masterfrom mahalex:fix-refactor-anonymous-types
Conversation
| ? list | ||
| .Replace( | ||
| list[^2], | ||
| list[^2].AsNode().WithAppendedTrailingTrivia(list[^1].GetTrailingTrivia())) |
There was a problem hiding this comment.
❔ Shouldn't this be leading and trailing trivia?
| list[^2].AsNode().WithAppendedTrailingTrivia(list[^1].GetTrailingTrivia())) | |
| list[^2].AsNode().WithAppendedTrailingTrivia(list[^1].GetLeadingTrivia()).WithAppendedTrailingTrivia(list[^1].GetTrailingTrivia())) |
There was a problem hiding this comment.
You are right! Fixed.
There was a problem hiding this comment.
By the way, currently some trivia of deleted parts in the initializer list is lost. For example,
var t1 = new
{
// Hi
a = 1,
b = 2
};
is transformed into
var t1 = new NewClass(
1,
2
);
Is this considered a bug?
There was a problem hiding this comment.
yup, i would def consider that a bug :)
There was a problem hiding this comment.
Preserving leading trivia from the initializers would also produce more reasonable indentation.
There was a problem hiding this comment.
yes. i woudl consider preservatin of leading trivia appropriate, as well as the removal of the final trailing trivia of the last element.
| return list.Count >= 2 && list[^1].IsToken | ||
| ? list | ||
| .Replace( | ||
| list[^2], | ||
| list[^2].AsNode() | ||
| .WithAppendedTrailingTrivia(list[^1].GetLeadingTrivia()) | ||
| .WithAppendedTrailingTrivia(list[^1].GetTrailingTrivia())) | ||
| .RemoveAt(list.Count - 1) | ||
| : list; |
There was a problem hiding this comment.
this has gotten to bee too much of a mouthful. Can you just have this be:
| return list.Count >= 2 && list[^1].IsToken | |
| ? list | |
| .Replace( | |
| list[^2], | |
| list[^2].AsNode() | |
| .WithAppendedTrailingTrivia(list[^1].GetLeadingTrivia()) | |
| .WithAppendedTrailingTrivia(list[^1].GetTrailingTrivia())) | |
| .RemoveAt(list.Count - 1) | |
| : list; | |
| if (list.Count % 2 == 1) | |
| return list; | |
| // now just return the list without the comma. |
There was a problem hiding this comment.
list.Count % 2 == 1 would break the empty list case, so we really need the list.Count >= 2 condition. But I agree that it's better to get rid of the ternary conditional.
| 2 | ||
| ); |
There was a problem hiding this comment.
| 2 | |
| ); | |
| 2 | |
| ); |
this doesn't seem desirable. If there is only whitespace trivia, i think it would be appropriate to just not transfer that along.
There was a problem hiding this comment.
Do you want to change the behavior in the no-trailing-comma case as well? Right now
var t1 = new
{
a = 1,
b = 2
};
is transformed into
var t1 = new NewClass(
1,
2
);
Also, other refactorings (like "Convert to tuple") have similar behavior, and I feel there should be consistency.
There was a problem hiding this comment.
i think there is a distinct difference int he patterns used when using { style versus (an initialization list) vs ( style (an argument list). I would prefer we matched the end style we expect people to most commonly use. That said, i'm ok with this being out of scope for this PR as we'd likely need to update a few things at the same time for consistency :)
There was a problem hiding this comment.
Agreed, I'll create an issue for that.
CyrusNajmabadi
left a comment
There was a problem hiding this comment.
Great thanks! LMK when this passes and i'll merge in for you :)
| : list; | ||
| if (list.Count == 0 || list.Count % 2 == 1) | ||
| { | ||
| return list; |
There was a problem hiding this comment.
💡
| return list; | |
| // The list is either empty, or does not end with a trailing comma | |
| return list; |
|
@CyrusNajmabadi looks like the build agents are feeling unwell. |
|
Thankjs for the PR! |
Fixes #45747