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

[BuildCheck] Editor config support #9811

Merged
merged 67 commits into from May 20, 2024
Merged

Conversation

f-alizada
Copy link
Contributor

@f-alizada f-alizada commented Mar 1, 2024

First iteration of the code with latest merged changes from exp/build-analyzers.

Design capturing current implementation could be viewed here: https://github.com/dotnet/msbuild/tree/dev/f-alizada/support-editorconfig/src/Build/BuildCheck/Infrastructure/EditorConfig

Discussions:

Current implementation.
The editorConfig parsing functionality is being used by https://github.com/dotnet/msbuild/blob/dev/f-alizada/support-editorconfig/src/Build/BuildCheck/Infrastructure/ConfigurationProvider.cs
to fetch all configs from .editorconfig.

  • Add comments on all public APIs
  • Check the case sensitive key-values in cache
  • Manual testing
    • unix
    • windows

Copy link
Member

@JanKrivanek JanKrivanek left a comment

Choose a reason for hiding this comment

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

First quick pass. Looks good! :-)
Though I'd love to see my static leftover being turned into mockable interface

I haven't reviewed the copied files though... (hopefully we'll anyways end up consuming a shared version from Roslyn)

Copy link
Member

@JanKrivanek JanKrivanek left a comment

Choose a reason for hiding this comment

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

I added couple additional comments

src/Analyzers.UnitTests/BuildAnalyzerConfiguration_Test.cs Outdated Show resolved Hide resolved
src/Analyzers.UnitTests/BuildAnalyzerConfiguration_Test.cs Outdated Show resolved Hide resolved
src/Analyzers.UnitTests/EditorConfig_Tests.cs Outdated Show resolved Hide resolved
src/Build/BuildCop/Infrastructure/ConfigurationProvider.cs Outdated Show resolved Hide resolved
src/Build/BuildCop/Infrastructure/ConfigurationProvider.cs Outdated Show resolved Hide resolved
src/Build/BuildCop/Infrastructure/ConfigurationProvider.cs Outdated Show resolved Hide resolved
@rainersigwald
Copy link
Member

Design capturing current implementation could be viewed here: https://github.com/dotnet/msbuild/tree/dev/f-alizada/support-editorconfig/src/Build/BuildCop/Infrastructure/EditorConfig

Can you add to this doc some details about what the exposed interface to this stuff is? That is, what interfaces/whatever MSBuild code will be using to learn about editorconfig-driven configuration?

Also, does this PR introduce the patterns that we'll use to map (stuff in the .editorconfig) to (severity for rules), or is that coming later?

Copy link
Member

@JanKrivanek JanKrivanek left a comment

Choose a reason for hiding this comment

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

Re-reviewed latest state - looks goot to go for initial version!

src/Analyzers.UnitTests/EndToEndTests.cs Outdated Show resolved Hide resolved
@f-alizada
Copy link
Contributor Author

f-alizada commented Mar 19, 2024

Can you add to this doc some details about what the exposed interface to this stuff is? That is, what interfaces/whatever MSBuild code will be using to learn about editorconfig-driven configuration?

Added example of the usage (API) and what it returns.

Also, does this PR introduce the patterns that we'll use to map (stuff in the .editorconfig) to (severity for rules), or is that coming later?

The PR introduced both -> .editorconfig parsing and mapping the configuration to the actual rules

Copy link
Member

@JanKrivanek JanKrivanek left a comment

Choose a reason for hiding this comment

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

I haver couple comments for consideration - but overall looks good!

@f-alizada f-alizada changed the title Editor config init [BuildCheck] Editor config support Apr 28, 2024
Copy link
Member

@JanKrivanek JanKrivanek left a comment

Choose a reason for hiding this comment

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

Thnak you for all the improvement roundtrips on the PR!

Copy link
Member

@ladipro ladipro 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!

@f-alizada f-alizada merged commit 352851b into main May 20, 2024
10 checks passed
@f-alizada f-alizada deleted the dev/f-alizada/support-editorconfig branch May 20, 2024 15:50
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.

None yet

5 participants