-
-
Notifications
You must be signed in to change notification settings - Fork 88
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
Basics of new override ability by extension, which will also allow other file extensions to be formatted. #1251
Conversation
…her file extensions to be formatted. closes #1220
245efe4
to
21bb5eb
Compare
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.
Bunch of suggestions, including whether partial overrides are supported.
); | ||
if (printerOptions is not null) | ||
{ | ||
printerOptions.IncludeGenerated = commandLineOptions.IncludeGenerated; |
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.
On an unrelated note, isn't it weird that everything else in CommandLineOptions is about control flow, and just this one is about formatting?
Should we maybe consider removing this option from CLI and have it only in the config files?
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 see it as similiar to directoryOrFile option dotnet csharpier <directoryOrFile>
which is used to decide which files/folders to format. Currently all the options in in the config file are about what to do when a file is formatted, as opposed to what files to format. Although the new overrides option allows you to include extensions that aren't normally included.
IncludeGenerated
does feel weird on PrinterOptions
. I couldn't really find a better place to put it.
Long term I think csharpier should transition to what prettier does, where <directoryOrFile>
uses globs. So the current command of dotnet csharpier .
would be the equivalent of dotnet csharpier **/*.{cs|cst}
directoryInfo = directoryInfo.Parent; | ||
|
||
while (directoryInfo is not null) | ||
if (filePath.EndsWith(".cs") || filePath.EndsWith(".csx")) |
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 check has come up in multiple places (including EditorConfigSections.cs
). Do you want to extract the extensions to a constant?
Or even create a helper method?
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 need to do something with it in https://github.com/belav/csharpier/pull/858/files, because that is going to add csproj/xml formatting. Maybe that is the time to switch the CLI option to use globs. I'll leave it for now and deal with it there, I'm sure there are plenty of conflicts on those lines already.
closes #1220
I mostly went with how prettier works for overrides. Using a file glob, a user can specify options to be used for a file. One of those options is which formatter to use. Right now this means non-standard files can be formatted with the csharp formatter. Some day the xml formatting PR will finally get some attention.