Skip to content

Add test demonstrating how trivia is handled#69448

Merged
CyrusNajmabadi merged 1 commit intodotnet:mainfrom
CyrusNajmabadi:arrayTest
Aug 10, 2023
Merged

Add test demonstrating how trivia is handled#69448
CyrusNajmabadi merged 1 commit intodotnet:mainfrom
CyrusNajmabadi:arrayTest

Conversation

@CyrusNajmabadi
Copy link
Contributor

No description provided.

@CyrusNajmabadi CyrusNajmabadi requested a review from a team as a code owner August 9, 2023 19:53
@ghost ghost added Area-IDE untriaged Issues and PRs which have not yet been triaged by a lead labels Aug 9, 2023
@CyrusNajmabadi CyrusNajmabadi requested a review from akhera99 August 9, 2023 19:54
Copy link
Contributor

@sharwell sharwell left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like the fixed code is missing a trailing comma in two cases.

[
// Leading
new[] { 1 }, // Trailing
new int[] { 2 }
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
new int[] { 2 }
new int[] { 2 },

[
// Leading
[1], // Trailing
[2]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
[2]
[2],

@CyrusNajmabadi
Copy link
Contributor Author

currently our features do not add a trailing comma, unless they're adding to an existing initializer that already had one.

I'm open to updating this decision and adding trailing commas in more places. However, i would like to do that outside of this PR.

@sharwell if we did go this route, would you be ok with these rules:

  1. if we are adding to an existing collection-initializer/expression, we will have a trailing comma or not, depending on what the original code had.
  2. If we are creating a new collection initializer/expression, and it all will be single-line, then we do not add a trailing comma.
  3. If we are creating a new collection initializer/expression and there would be multiple elements, we place a comma after the last one.

If that fits what you want, i can make a followup PR to encode those rules.

Thanks!

@sharwell
Copy link
Contributor

sharwell commented Aug 9, 2023

Yes, those are exactly the expected rules. I believe we settled on them in an earlier discussion.
#54859 (comment)

We should prioritize that work, as the current refactoring produces code which is problematic for maintainability.

@CyrusNajmabadi
Copy link
Contributor Author

We should prioritize that work, as the current refactoring produces code which is problematic for maintainability.

Sure. I'll do tomorrow.

@CyrusNajmabadi
Copy link
Contributor Author

@akhera99 @sharwell for approval.

@CyrusNajmabadi CyrusNajmabadi merged commit efe129c into dotnet:main Aug 10, 2023
@CyrusNajmabadi CyrusNajmabadi deleted the arrayTest branch August 10, 2023 17:25
@ghost ghost added this to the Next milestone Aug 10, 2023
@dibarbet dibarbet modified the milestones: Next, 17.8 P2 Aug 28, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Area-IDE untriaged Issues and PRs which have not yet been triaged by a lead

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants