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

Open
wants to merge 59 commits into
base: main
Choose a base branch
from

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/BuildCop/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#L129
to fetch all configs from .editorconfig.

Retrieve infra configuration for rule:

internal BuildAnalyzerConfiguration GetUserConfiguration(string projectFullPath, string ruleId)

  1. Fetch all key-value pairs from editorconfig files for file.
  2. Filter out non infra related keys
  3. Build configuration instance

Retrieve non infra configuration for rule:

public CustomConfigurationData GetCustomConfiguration(string projectFullPath, string ruleId)

  1. Fetch all key-value pairs from editorconfig files for file.
  2. Filter out infra related keys
  3. Build CustomConfigrationData
  • 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

@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.

Apologies for the delay. Looks great overall, I am leaving a few inline comments.

{
var editorConfigfileContent = File.ReadAllText(editorConfigFilePath);
editorConfig = EditorConfigFile.Parse(editorConfigfileContent);
editorConfigFileCache[editorConfigFilePath] = editorConfig;
Copy link
Member

Choose a reason for hiding this comment

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

Is it Ok for this class to not be thread safe?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Discussed offline with @JanKrivanek and there is a case with graph enabled scenario where it could be used like that. So update it to the use ConcurrentDictionary.

src/Build/BuildCheck/API/BuildAnalyzerConfiguration.cs Outdated Show resolved Hide resolved
@ladipro ladipro self-requested a review April 23, 2024 10:19
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!

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