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

feat: Added ConfigFile to simplify AnalyzerConfigFile creation. #981

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

Conversation

HavenDV
Copy link

@HavenDV HavenDV commented Apr 18, 2022

I recently got acquainted with your package and for me creating a configuration file was problematic until I found an example. I think this can simplify the entry threshold because it adds a specific Add method that will be used intuitively.

@HavenDV HavenDV requested a review from a team as a code owner April 18, 2022 19:53
@sharwell
Copy link
Member

I'm a bit hesitant to add this here in the current form. Note that you can achieve the same outcome by defining the Add method as an extension method in your own project.

Here are issues I'd like to see addressed in a general API:

  1. Allow adding /.editorconfig or /.globalconfig without specifying the file name directly.
  2. Avoid having the new Add method apply to ProjectState.Sources, ProjectState.AdditionalFiles, or ProjectState.GeneratedSources.
  3. Avoid the need to type new Dictionary<string, Dictionary<string, string>> or new Dictionary<string, string>, similar to how we avoid the need to type new ProjectState(projectName, language, $"/{projectName}/Test", extension) when adding items to SolutionState.AdditionalProjects.

Copy link
Member

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

See notes in review

@HavenDV
Copy link
Author

HavenDV commented Apr 20, 2022

Thank you. Tried to take into account your comments.
The only downside I don't like about the new implementation is the duplication of the section key here if you need to add multiple values to the same section.

var actual = new EditorConfigFile
{
    ["root"] = "true",
    ["*", "key"] = "{|Literal:value|}",
}.ToString();

@sharwell
Copy link
Member

The only downside I don't like about the new implementation is the duplication of the section key here if you need to add multiple values to the same section.

I did notice this. I believe you could resolve this by making root be a constructor parameter:

var actual = new EditorConfigFile(root: true)
{
  ["*"] =
  {
    "key = {|Literal:value|}",
  },
}.ToString();

or:

var actual = new EditorConfigFile(root: true)
{
  ["*"] =
  {
    { "key", "{|Literal:value|}" },
  },
}.ToString();

@HavenDV
Copy link
Author

HavenDV commented Apr 21, 2022

Perhaps I do not understand something, but how to specify global values other than root, which can be anything?

Will the values for the * section be available via AnalyzerConfigOptionsProvider.GlobalOptions?

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.

2 participants