-
-
Notifications
You must be signed in to change notification settings - Fork 91
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 support for editorconfig #927
Conversation
I still need to update the doc, but this is ready for review..... actually looks like there are some test failures. Possibly a linux vs windows path issue. |
@shocklateboy92 this was ready for review if you wanted to take a gander. It is a bigger change so I didn't want to skip over your watchful eye. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I haven't read through the whole PR yet, but I figured the one comment I had so far was important enough to commit before going to bed tonight 🙂
|
||
internal static class ConfigFileParser | ||
{ | ||
private static readonly Regex SectionRegex = new(@"^\s*\[(([^#;]|\\#|\\;)+)\]\s*([#;].*)?$"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So, I'm not a super big fan of parsing source using regex.
People have built libraries that use actual parsers and lexers to handle all the weird corner cases properly.
editorconfig.org recommends:
https://github.com/editorconfig/editorconfig-core-net#readme
However, editorconfig is actually an INI file and there are plenty of well known libraries for those.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was originally using that project, but it didn't support IFileSystem - https://github.com/editorconfig/editorconfig-core-net/pulls
I also realized it was built to find a editorconfig for a given file, which would be too slow for what csharpier needs.
That regex comes out of editorconfig-core-net, I did find https://github.com/rickyah/ini-parser just now. I will see if it is easy to pull in.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay, I actually did have time to finish the review tonight (finally).
Using ini-parser cleaned up the code and should do a much better job of parsing the files correctly. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
minor nit: feel free to complete this PR yourself without waiting for another review if you choose to address it.
var sections = new List<Section>(); | ||
foreach (var section in configData.Sections) | ||
{ | ||
sections.Add(new Section(section, directory)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: I feel like this whole loop could have been a .Select()
.
} | ||
|
||
var directoryInfo = fileSystem.DirectoryInfo.FromDirectoryName(directoryName); | ||
var editorConfigFiles = directoryInfo |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thus only a part of the .editorconfig
of the project is considered. Since the project system / Visual Studio with <EditorConfigFiles />
allows to load further files. In addition Visual Studios extension the Global AnalyzerConfig
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This comes from the doc on AnalyzerConfig, so I don't think there need to be any changes to support it.
global configuration files can't be used to configure editor style settings for IDEs, such as indent size or whether to trim trailing whitespace. Instead, they are designed purely for specifying project-level analyzer configuration options.
I wasn't aware of EditorConfigFiles
but I'll look into it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It sounds like usage of EditorConfigFiles
is not recommended and Global AnalyzerConfig
should be used instead. https://github.com/MicrosoftDocs/visualstudio-docs/issues/6051#issuecomment-1092262347 Which makes it sounds like this is more intended to be used for configuring analyzers.
There is an easy workaround of just adding an .editorconfig
or .csahrpierrc
to the root, so it doesn't seem critical to support EditorConfigFiles
.
closes #630