Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Adding a setting to .editorconfig to enforce formatting #11236

Merged
merged 3 commits into from
Nov 21, 2024

Conversation

alexgav
Copy link
Contributor

@alexgav alexgav commented Nov 21, 2024

Summary of the changes

  • Adding .editorconfig setting to flag formatting issues as warnings during build

Fixes: https://github.com/orgs/dotnet/projects/197?pane=issue&itemId=88054220

@alexgav alexgav requested a review from a team as a code owner November 21, 2024 01:14
Comment on lines 147 to 148
where !keyValuePair.Key.TranslateTo(visibleSpan.Snapshot, SpanTrackingMode.EdgeExclusive).IntersectsWith(visibleSpan)
select keyValuePair.Key;
Copy link
Contributor

Choose a reason for hiding this comment

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

What does this end up looking like if we format it? I can't see anything obviously weird here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

C# formatting bug?

image

Copy link
Contributor

Choose a reason for hiding this comment

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

that looks like a formatting bug...

what if we change it to

var toRemove =
	from keyValuePari
	in _adornmentCache
	where ....
	select ...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

... unless I manually indent in, then formatter is happy with this

image

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Looking at the docs at https://learn.microsoft.com/en-us/dotnet/csharp/language-reference/keywords/from-clause, recommendation seems to be to have in on the same line as from, after which formatting basically makes sense (though formatting in the first two examples in the same docs is still getting "corrected" by C# formatter). I made the change to make the formatter happy.

Copy link
Contributor

Choose a reason for hiding this comment

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

ahh, yep, that's what it is. I didn't see it, but I always have put in on the same line as from.

Copy link
Member

@DustinCampbell DustinCampbell Nov 21, 2024

Choose a reason for hiding this comment

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

ahh, yep, that's what it is. I didn't see it, but I always have put in on the same line as from.

FWIW, that's pretty uncommon. Every single LINQ sample ever produced by Microsoft had from and in on the same line because that's how we expected query expressions to be formatted. I've personally never seen in on a separate line until now. 😄

Copy link
Contributor

Choose a reason for hiding this comment

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

FWIW, that's pretty uncommon

I think you misread what I wrote :) We're in vehement agreement about from and in being on the same line :)

I've personally never seen in on a separate line until now

Me neither, that's why I missed it when I wrote "I can't see anything obviously weird here" above.

@@ -8,9 +8,11 @@ namespace Microsoft.AspNetCore.Razor.LanguageServer.Hosting;
[Flags]
internal enum FormattingFlags
{
#pragma warning disable format
Copy link
Contributor

Choose a reason for hiding this comment

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

any particular reason? What does it want to format to?

Copy link
Contributor Author

@alexgav alexgav Nov 21, 2024

Choose a reason for hiding this comment

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

It wants to remove all spaces but 1. I am not seeing an option to allow multiple spaces in Flags. I mean we don't have to have them, just looks nicer :) This is what it would've looked like

image

Copy link
Contributor

Choose a reason for hiding this comment

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

Personally, I'm totally fine with this not being lined up perfectly. I really don't like suppressions, they're uglier than jagged formatting :D

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's a fair point. I removed suppressions and extra spaces. I think all suppressions are gone now.

Copy link
Member

Choose a reason for hiding this comment

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

IMO, if we want this setting in .editorconfig, we should agree to never suppress it.

Comment on lines 147 to 148
where !keyValuePair.Key.TranslateTo(visibleSpan.Snapshot, SpanTrackingMode.EdgeExclusive).IntersectsWith(visibleSpan)
select keyValuePair.Key;
Copy link
Contributor

Choose a reason for hiding this comment

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

that looks like a formatting bug...

what if we change it to

var toRemove =
	from keyValuePari
	in _adornmentCache
	where ....
	select ...

@alexgav alexgav enabled auto-merge (squash) November 21, 2024 09:58
@alexgav alexgav merged commit fa5b57f into main Nov 21, 2024
12 checks passed
@alexgav alexgav deleted the dev/alexgav/EnforceCorrectFormatting branch November 21, 2024 10:24
@dotnet-policy-service dotnet-policy-service bot added this to the Next milestone Nov 21, 2024
@jjonescz jjonescz modified the milestones: Next, 17.13 P2 Nov 25, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants