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

Is there a way to run dotnet-format with --fix-style CLI argument? #1642

Closed
lextatic opened this issue Jul 23, 2022 · 18 comments
Closed

Is there a way to run dotnet-format with --fix-style CLI argument? #1642

lextatic opened this issue Jul 23, 2022 · 18 comments
Labels
question Further information is requested

Comments

@lextatic
Copy link
Contributor

I want to lint C# with Naming rules. That would be possible if I could use the parameter --fix-style with the MegaLinter's dotnet-format (dotnet 5.0). Unfortunately this doesn't seem to work because I think the default command is always executed with the --folder parameter which is mutually exclusive with the --fix-style parameter.

--fix-style needs to run considering a workspace.
--folder runs for individual files.

Am I missing something? Is this even possible? Should I open a feature request?

@lextatic lextatic added the question Further information is requested label Jul 23, 2022
@lextatic lextatic changed the title Is there a way to run dotnet-format with --fix-style parameter? Is there a way to run dotnet-format with --fix-style CLI argument? Jul 23, 2022
@Kurt-von-Laven
Copy link
Collaborator

I am struggling to find official documentation on --fix-style. Would you be able to point me to in the right direction? My team grew impatient with the performance of dotnet-format, so we disabled it and instead run CSharpier manually. We feel it produces considerably better results, and it is objectively dramatically faster.

@nvuillam
Copy link
Member

Maybe we should switch all "file by file" linters into project mode by default... :)
Please provide the dotnet-format command line as you need it and we'll see what we can do :)

@Kurt-von-Laven
Copy link
Collaborator

When VALIDATE_ALL_CODEBASE is true, that makes sense to me (at least for linters that perform better in project mode), but it would slow incremental runs down greatly.

@nvuillam
Copy link
Member

nvuillam commented Jul 24, 2022

I use incremental run on none of my projects, I want legacy-not-updated code to also be checked everytime with latest versions of linters 👿

@nvuillam
Copy link
Member

But yes we could something like "if incremental then file by file, else project mode" :)

@lextatic
Copy link
Contributor Author

lextatic commented Jul 24, 2022

I am struggling to find official documentation on --fix-style. Would you be able to point me to in the right direction? My team grew impatient with the performance of dotnet-format, so we disabled it and instead run CSharpier manually. We feel it produces considerably better results, and it is objectively dramatically faster.

Yea, dotnet-format's documentation isn't very clear/consistent, I'm just trying to figure it out myself. Good thing is I think I did, I'll explain what I found out later in this post.

About CSharpier, I haven't played with it yet. It seems sleek and nice. Why not open a feature request for adding this linter? The thing is it uses dotnet 6.0 and I think MegaLinter is currently running on dotnet 5.0.

Maybe we should switch all "file by file" linters into project mode by default... :) Please provide the dotnet-format command line as you need it and we'll see what we can do :)

Maybe you should, but I'm not sure it would be necessary. What I think is needed is to leave out the --folder arg and add a new optional variable to point at where the workspace is when running CSHARP_DOTNET_FORMAT_CLI_LINT_MODE: project.

With dotnet 5.0 the command should be:
dotnet format <workspace> [options]

and with dotnet 6.0 (which is not the case for MegaLinter, but should you upgrade it someday):
dotnet format [command] <workspace> [options]

btw, --folder arg doesn't even exist when running dotnet 6.0.

For example, let's say I have this file structure:

[ ] root
 |
 `--[ ] dotnet-project
     |
     |-- .editorconfig
     |-- solution.sln
     |
     |--[ ] project1 that is referenced in solution.sln
     |   |
     |   |-- project1.csproj
     |   |-- Class1.cs
     |   `-- Class2.cs
     |
     |--[ ] project2 that is also referenced in solution.sln
     |   |
     |   |-- project2.csproj
     |   |-- Class3.cs
     |   `-- Class4.cs
     |
     `--[ ] project3 that is not referenced in solution.sln
         |
         |-- project3.csproj
         |-- Class5.cs
         `-- Class6.cs

On dotnet 5.0 if I'm executing commands from root I need to specify a <workspace>. It also runs with whitespace fixes by default, but not style fixing, so I would have to be very specific if I want one or another.
dotnet format dotnet-project -w -s warn

But since project3 is not referenced in solution.sln, Class5.cs and Class6.cs won't be linted. Nothing much we can do here, we just have to run again with:
dotnet format dotnet-project\project3 -w -s warn

As you can see the <workspace> arg can point to a folder containing either a .sln file or a .csproj file, but only files referenced on them will be linted. Dotnet-format figures out which .editorconfig to use automatically if it's located along with those files, in this case even when specifying project3.csproj as the workspace it will use the .editorconfig on dotnet-project folder.

If I were running the command on dotnet-project folder or there were a .sln or .csproj file on root I could leave out the <workspace> argument entirely like this:
dotnet format -w -s warn

Now for incremental run you could use the --include arg like this:
dotnet format dotnet-project -w -s warn --include project1\Class1.cs project2\Class3.cs project3\Class5.cs

In dotnet 5.0 file path should be relative to workspace. Only Class1.cs and Class3.cs would be linted as Class5.cs is not referenced in the original solution.sln file.

Now, as for dotnet 6.0 the <workspace> works the same way, but the default execution will always fix whitespaces and style (those being controlled by the command args), and the file path is always relative to the root folder (or relative to where you're executing the command).

So:

dotnet format dotnet-project
would lint both whitespaces and style for Class1.cs, Class2.cs, Class3.cs and Class4.cs.

dotnet format whitespace dotnet-project
would lint only whitespaces for Class1.cs, Class2.cs, Class3.cs and Class4.cs.

dotnet format dotnet-project --include dotnet-project\project1\Class1.cs dotnet-project\project2\Class3.cs dotnet-project\project3\Class5.cs
would lint both whitespaces and style for Class1.cs and Class3.cs.

And last but not least, as with the dotnet 5.0, if I were running the command on dotnet-project folder or there were a .sln or .csproj file on root I could leave out the <workspace> argument entirely like this:

dotnet format
or
dotnet format --include <files>
or even
dotnet format style --include <files>

@lextatic
Copy link
Contributor Author

@Kurt-von-Laven it seems CSharpier only apply whitespaces fixes and such. When using dotnet-format with --fix-style or -s command it can apply Naming rules configured in the .editorconfig file. I think it's a lot more powerful as it can even check naming conventions for your variables, like for example forcing _camelCase for private fields, or any other totally customizable rules you come up with.

Those are specified here and you can play with Visual Studio to make it generate a file with those settings for you in Tools > Options then Text Editor > C# > Code Style > Generate .editorconfig file from settings.

Also, in the Naming section you can create specifications and naming styles and they're all considered by the dotnet-format tool if present in the .editorconfig file. Even though it can't always fix those mistakes it'll report them when running the --fix-style.

@nvuillam
Copy link
Member

MegaLinter works best with list_of_files, and I like to have latest versions of platforms, so I try some things in this PR -> #1646

@lextatic
Copy link
Contributor Author

lextatic commented Jul 25, 2022

MegaLinter works best with list_of_files, and I like to have latest versions of platforms, so I try some things in this PR -> #1646

I think you forgot to remove the --folder arg from the descritors. Also --check should probably be replaced with --verify-no-changes.

@lextatic
Copy link
Contributor Author

lextatic commented Jul 25, 2022

@nvuillam Oh well, you might want to check out this issue from dotnet format repo.

It looks like the default linter now requires a .csproj or .sln file to properly run, so it probably won't run with just a list_of_files as it used to unless you use it like this:
dotnet format whitespace --folder --verify-no-changes --exclude / --include myfile.cs myfile2.cs

With that you might want to ignore my PR comment telling you to remove the --folder arg and instead add a whitespace arg before that.

Unfortunately that doesn't solve this issue as using the whitespace argument won't run style fixing.

======

A more complete solution would be:

Have a variable CSHARP_DOTNET_FORMAT_CLI_WORSKPACE to work as the <workspace> argument to indicate the path to the .sln or .csproj file. Default value is ./ and if not set adds the --folder argument to the CLI run so dotnet format won't ask for a .sln or .csproj file.

Have a variable CSHARP_DOTNET_FORMAT_LINT_MODE to work as the [command] argument. Possible values are whitespace, style and both. (both actually removes this argument from CLI, leaving it empty).

Have it so that when a value is set for CSHARP_DOTNET_FORMAT_LINT_MODE, a value for CSHARP_DOTNET_FORMAT_CLI_WORSKPACE must also be provided. This grantees that --folder will be safely removed with a workspace specified. (maybe this shouldn't be enforced, but documented instead)

Just for review, the command should be:
dotnet format [command] <workspace> [options]

The default would be to run as a list_of_files like this:
dotnet format whitespace ./ --folder --verify-no-changes --exclude / --include myfile.cs myfile2.cs

If CSHARP_DOTNET_FORMAT_LINT_MODE is set to style then CSHARP_DOTNET_FORMAT_CLI_WORSKPACEmust be set:

  CSHARP_DOTNET_FORMAT_LINT_MODE: style
  CSHARP_DOTNET_FORMAT_CLI_WORSKPACE: "./"

which give us:
dotnet format style ./ --verify-no-changes --exclude / --include myfile.cs myfile2.cs

If CSHARP_DOTNET_FORMAT_LINT_MODE is set to none then CSHARP_DOTNET_FORMAT_CLI_WORSKPACEmust be set:

  CSHARP_DOTNET_FORMAT_LINT_MODE: none
  CSHARP_DOTNET_FORMAT_CLI_WORSKPACE: "./"

which give us:
dotnet format ./ --verify-no-changes --exclude / --include myfile.cs myfile2.cs

Sorry for all this inconvenience, I would make a PR myself but I'm not familiarized enough with your repo (nor python).

PS: I used CSHARP_DOTNET_ as an example but those would apply to VBDOTNET_DOTNET_ as well.

Edit: A clever solution would be to make CSHARP_DOTNET_FORMAT_CLI_WORSKPACE default value as --folder. Which should replace it with a path in the correct position on CLI when set if it's set as the first arg, no need for any extra work.

@github-actions
Copy link
Contributor

This issue has been automatically marked as stale because it has not had recent activity.
It will be closed in 14 days if no further activity occurs.
Thank you for your contributions.

If you think this issue should stay open, please remove the O: stale 🤖 label or comment on the issue.

@github-actions github-actions bot added the O: stale 🤖 This issue or pull request is stale, it will be closed if there is no activity label Aug 25, 2022
@github-actions github-actions bot closed this as completed Sep 9, 2022
@nvuillam nvuillam reopened this Sep 9, 2022
@github-actions github-actions bot removed the O: stale 🤖 This issue or pull request is stale, it will be closed if there is no activity label Sep 10, 2022
@github-actions
Copy link
Contributor

This issue has been automatically marked as stale because it has not had recent activity.
It will be closed in 14 days if no further activity occurs.
Thank you for your contributions.

If you think this issue should stay open, please remove the O: stale 🤖 label or comment on the issue.

@github-actions github-actions bot added the O: stale 🤖 This issue or pull request is stale, it will be closed if there is no activity label Oct 10, 2022
@github-actions github-actions bot closed this as not planned Won't fix, can't repro, duplicate, stale Oct 24, 2022
@nvuillam nvuillam removed the O: stale 🤖 This issue or pull request is stale, it will be closed if there is no activity label Oct 24, 2022
@Kurt-von-Laven Kurt-von-Laven reopened this Nov 1, 2022
@Kurt-von-Laven
Copy link
Collaborator

Similar to #1994.

@github-actions
Copy link
Contributor

github-actions bot commented Dec 2, 2022

This issue has been automatically marked as stale because it has not had recent activity.
It will be closed in 14 days if no further activity occurs.
Thank you for your contributions.

If you think this issue should stay open, please remove the O: stale 🤖 label or comment on the issue.

@github-actions github-actions bot added the O: stale 🤖 This issue or pull request is stale, it will be closed if there is no activity label Dec 2, 2022
@nvuillam nvuillam removed the O: stale 🤖 This issue or pull request is stale, it will be closed if there is no activity label Dec 2, 2022
@github-actions
Copy link
Contributor

github-actions bot commented Jan 2, 2023

This issue has been automatically marked as stale because it has not had recent activity.
It will be closed in 14 days if no further activity occurs.
Thank you for your contributions.

If you think this issue should stay open, please remove the O: stale 🤖 label or comment on the issue.

@github-actions github-actions bot added the O: stale 🤖 This issue or pull request is stale, it will be closed if there is no activity label Jan 2, 2023
@nvuillam nvuillam removed the O: stale 🤖 This issue or pull request is stale, it will be closed if there is no activity label Jan 2, 2023
@nvuillam
Copy link
Member

nvuillam commented Jan 7, 2023

@lextatic is this issue still relevant after your dotnet v6 PR ?

@lextatic
Copy link
Contributor Author

lextatic commented Jan 9, 2023

It's not. With the upgrade dotnet-format v6 defaults to linting with style included.

@lextatic lextatic closed this as completed Jan 9, 2023
@nvuillam
Copy link
Member

nvuillam commented Jan 9, 2023

Thanks for the feedback :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
question Further information is requested
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants