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.

Sign up for GitHub

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

Jump to bottom

Move formatting analyzer to Analyzers layer #60549

Merged
merged 14 commits into from
Apr 7, 2022
Merged

Move formatting analyzer to Analyzers layer #60549

merged 14 commits into from
Apr 7, 2022

Conversation

Youssef1313
Copy link
Member

@Youssef1313 Youssef1313 commented Apr 3, 2022 •
edited
Loading

There were two versions of formatting analyzer. One in Features and the other is in CodeStyle. This PR makes a single analyzer in Analyzers layer, which is shared between both CodeStyle and Features.

@Youssef1313 Youssef1313 requested a review from a team as a code owner April 3, 2022 22:27
@ghost ghost added the Community The pull request was submitted by a contributor who is not a Microsoft employee. label Apr 3, 2022
@Youssef1313 Youssef1313 requested a review from a team as a code owner April 3, 2022 22:40
@Youssef1313 Youssef1313 mentioned this pull request Apr 3, 2022
Closed
Youssef1313
Youssef1313 commented Apr 3, 2022
View reviewed changes
src/Analyzers/Core/Analyzers/Formatting/FormattingAnalyzerHelper.cs
Comment on lines -11 to -16
#if CODE_STYLE
using Formatter = Microsoft.CodeAnalysis.Formatting.FormatterHelper;
using FormattingProvider = Microsoft.CodeAnalysis.Formatting.ISyntaxFormatting;
#else
using FormattingProvider = Microsoft.CodeAnalysis.Host.HostWorkspaceServices;
#endif
Copy link
Member Author

Choose a reason for hiding this comment

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

Previously, Features was using Mef and CodeStyle was using ISyntaxFormatting. Now both are using ISyntaxFormatting. @mavasani Can you confirm this is the right action?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think it should be fine, @sharwell to confirm

Youssef1313
Youssef1313 commented Apr 4, 2022
View reviewed changes
src/Analyzers/Core/CodeFixes/Formatting/FormattingCodeFixProvider.cs
@@ -28,12 +26,14 @@ public sealed override Task RegisterCodeFixesAsync(CodeFixContext context)
{
foreach (var diagnostic in context.Diagnostics)
{
#pragma warning disable RS0005 // Do not use generic 'CodeAction.Create' to create 'CodeAction'
context.RegisterCodeFix(
Copy link
Member Author

Choose a reason for hiding this comment

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

@mavasani The one that used to live in Features was inheriting SyntaxEditorBasedCodeFixProvider. While the one that lived in CodeStyle was inheriting CodeFixProvider directly. Does it matter what do I inherit when merging both in Analyzers layer?

Copy link
Contributor

Choose a reason for hiding this comment

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

Moving to SyntaxEditorBasedCodeFixProvider is preferable if possible, and you don't see any breaking changes.

Copy link
Member Author

Choose a reason for hiding this comment

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

@mavasani Done in last commit (5e4b835)

@Youssef1313
Copy link
Member Author

@mavasani Is this ready to merge?

@mavasani
Copy link
Contributor

mavasani commented Apr 6, 2022

@Youssef1313 There are merge conflicts

@Youssef1313
Copy link
Member Author

@mavasani I resolved the conflicts.

@CyrusNajmabadi
Copy link
Member

This is AWESOME. Thanks so much @Youssef1313 !

mavasani
mavasani approved these changes Apr 6, 2022
View reviewed changes
Copy link
Contributor

@mavasani mavasani left a comment

Choose a reason for hiding this comment

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

Thank you @Youssef1313

@mavasani mavasani enabled auto-merge April 6, 2022 16:45
@Youssef1313
Copy link
Member Author

Can someone restart the failing tests please?

@CyrusNajmabadi
Copy link
Member

Done!

auto-merge was automatically disabled April 6, 2022 19:37

Head branch was pushed to by a user without write access

@Youssef1313
Copy link
Member Author

@CyrusNajmabadi I fixed merge conflicts so auto merge got disabled. Can you enable it again? Thanks!

@CyrusNajmabadi
Copy link
Member

Done!

@CyrusNajmabadi CyrusNajmabadi merged commit f8a1185 into dotnet:main Apr 7, 2022
@ghost ghost added this to the Next milestone Apr 7, 2022
@Youssef1313 Youssef1313 deleted the formatting branch April 7, 2022 06:31
mavasani
mavasani reviewed Apr 9, 2022
View reviewed changes
src/CodeStyle/CSharp/Tests/Microsoft.CodeAnalysis.CSharp.CodeStyle.UnitTests.csproj
<ProjectReference Include="..\..\..\Compilers\Test\Core\Microsoft.CodeAnalysis.Test.Utilities.csproj" />
<ProjectReference Include="..\..\..\Workspaces\CoreTestUtilities\Microsoft.CodeAnalysis.Workspaces.Test.Utilities.csproj" />
</ItemGroup>
<Import Project="..\..\..\Analyzers\CSharp\Tests\CSharpAnalyzers.UnitTests.projitems" Label="Shared" />
Copy link
Contributor

Choose a reason for hiding this comment

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

@Youssef1313 My bad that I did not notice that your change actually deletes a bunch of test projects. C# and VB CodeStyle unit test projects test the analyzers that ship as part of respective CodeStyle NuGet package. I am going to file an issue to restore these projects back.

Copy link
Member Author

Choose a reason for hiding this comment

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

Oops. Yeah sorry for that.

@mavasani mavasani mentioned this pull request Apr 9, 2022
Closed
@dibarbet dibarbet modified the milestones: Next, 17.3.P1 Apr 25, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mavasani mavasani mavasani approved these changes

Assignees

@sharwell sharwell

Labels
Area-IDE Community The pull request was submitted by a contributor who is not a Microsoft employee.
Projects
None yet
Milestone
17.3.P1
Development

Successfully merging this pull request may close these issues.
5 participants
@Youssef1313 @mavasani @CyrusNajmabadi @sharwell @dibarbet