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.

Sign up for GitHub

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

Jump to bottom

--check returns exit code 2 when no files are changed #979

Open
OneCyrus opened this issue Feb 16, 2021 · 21 comments
Open

--check returns exit code 2 when no files are changed #979

OneCyrus opened this issue Feb 16, 2021 · 21 comments
Labels
Feature Request This issue is requesting an enhancement or new feature

Comments

@OneCyrus
Copy link

the --check argument returns exit code 2 when there are some warnings from the analyzers which aren't related to formatting. --check should always return a 0 when no changes are made to files so we can use it in CI pipelines.
@jmarolf
Copy link
Contributor

jmarolf commented Feb 16, 2021

I think we should definitely add a "allow warnings" option that does this. I could also see someone making a change to the project file that causes dotnet format to not be able to apply things and you would want to know about that

@JoeRobich
Copy link
Member

@OneCyrus Could you share some of the diagnostics warnings that were causing you issues? If the issue is with non-fixable compiler diagnostics, then hopefully #981 will resolve this for you.

@JoeRobich JoeRobich added the Needs More Info This issue needs additional details to be actionable label Feb 16, 2021
@OneCyrus
Copy link
Author

there are all kind of warnings showing up which are not related to formatting. just a short part of the logs. but yes, there are also non-fixable compiler diagnostics warnings. do you think only the non-fixable diagnostics are the problem?

GraphQL/Shared/DataSet/DataSet.cs(10,23): Non-nullable property 'Name' must contain a non-null value when exiting constructor. Consider declaring the property as nullable. (CS8618)
GraphQL/Shared/DataSet/DataSet.cs(11,23): Non-nullable property 'Unit' must contain a non-null value when exiting constructor. Consider declaring the property as nullable. (CS8618)
Schema/Network/NetworkPerformanceCounterModel.cs(15,23): Non-nullable property 'Location' must contain a non-null value when exiting constructor. Consider declaring the property as nullable. (CS8618)
Schema/Network/NetworkPerformanceCounterModel.cs(17,23): Non-nullable property 'HostName' must contain a non-null value when exiting constructor. Consider declaring the property as nullable. (CS8618)

@JoeRobich
Copy link
Member

do you think only the non-fixable diagnostics are the problem?

Diagnostics which begin with CS typically are compiler diagnostics. Could you install a nightly build (these are the instructions), which contains the fix that I mentioned earlier, and let us know if that resolves the issue for you?

@OneCyrus
Copy link
Author

just tried with version 6.0.211701 and it gives also exit code 2. i guess it really reports all rule violations as errors.

@OneCyrus
Copy link
Author

just did a simple test with a single rule violation. is this expected? i guess "format" shouldn't report on linting rules. IDE1006 results in exit code 2.

PS /home/username/source/SelfService/GIA.WebServices.EndpointMgnt> dotnet format --fix-analyzers info --fix-style info --fix-whitespace -v diag --check
  The dotnet format version is '6.0.211701+1f174eaed9acec98641473e579a24ed7b0753590'.
  The dotnet runtime version is '5.0.2'.
  The dotnet CLI version is '5.0.102'.
  Using MSBuild.exe located in '/usr/share/dotnet/sdk/5.0.102/'.
  Formatting code files in workspace '/home/username/source/SelfService/GIA.WebServices.EndpointMgnt/GIA.WebServices.EndpointMgnt.sln'.
  Loading workspace.
  Complete in 2661ms.
  Determining formattable files.
  Complete in 360ms.
  Running formatters.
  Running Code Style analysis.
  Determining diagnostics...
  Running 46 analyzers on GIA.WebServices.EndpointMgnt.
  Running 46 analyzers on GIA.WebServices.EndpointMgnt.Tests.
  GIA.WebServices.EndpointMgnt.Tests/IntegrationTests.cs(64,50): Naming rule violation: Missing prefix: 's_' (IDE1006)
  Complete in 2523ms.
  Analysis complete in 2523ms.
  Running Analyzer Reference analysis.
  Determining diagnostics...
  Running 171 analyzers on GIA.WebServices.EndpointMgnt.
  Running 208 analyzers on GIA.WebServices.EndpointMgnt.Tests.
  Complete in 617ms.
  Analysis complete in 617ms.
  Complete in 3753ms.
  Formatted code file '/home/username/source/SelfService/GIA.WebServices.EndpointMgnt/GIA.WebServices.EndpointMgnt.Tests/IntegrationTests.cs'.
  Formatted 1 of 26 files.
  Format complete in 6774ms.
PS /home/username/source/SelfService/GIA.WebServices.EndpointMgnt> $lastexitcode                                                                       
2

PS /home/username/source/SelfService/GIA.WebServices.EndpointMgnt> dotnet format --fix-analyzers info --fix-style info --fix-whitespace -v diag --check
  The dotnet format version is '6.0.211701+1f174eaed9acec98641473e579a24ed7b0753590'.
  The dotnet runtime version is '5.0.2'.
  The dotnet CLI version is '5.0.102'.
  Using MSBuild.exe located in '/usr/share/dotnet/sdk/5.0.102/'.
  Formatting code files in workspace '/home/username/source/SelfService/GIA.WebServices.EndpointMgnt/GIA.WebServices.EndpointMgnt.sln'.
  Loading workspace.
  Complete in 2456ms.
  Determining formattable files.
  Complete in 380ms.
  Running formatters.
  Running Code Style analysis.
  Determining diagnostics...
  Running 46 analyzers on GIA.WebServices.EndpointMgnt.
  Running 46 analyzers on GIA.WebServices.EndpointMgnt.Tests.
  Complete in 2650ms.
  Analysis complete in 2650ms.
  Running Analyzer Reference analysis.
  Determining diagnostics...
  Running 171 analyzers on GIA.WebServices.EndpointMgnt.
  Running 208 analyzers on GIA.WebServices.EndpointMgnt.Tests.
  Complete in 611ms.
  Analysis complete in 611ms.
  Complete in 3851ms.
  Formatted 0 of 26 files.
  Format complete in 6688ms.
PS /home/username/source/SelfService/GIA.WebServices.EndpointMgnt> $lastexitcode                                                                       
0

@JoeRobich
Copy link
Member

This behavior is expected. Since you are passing the --fix-style option, code style issue will now fail the check. Similarly, passing --fix-analyzers means that issues reported by 3rd party analyzers will fail the check. If you wish to only fail on whitespace formatting issues, then dotnet format -v diag --check is the command you want.

@OneCyrus
Copy link
Author

that's unfortunate as we would like to fix all style and analyzer issues too. there should be an option which checks if dotnet format makes actual changes to files instead of reporting compiler errors/warnings. isn't that the job of dotnet build/msbuild to break on compiler validation? i expected dotnet format to auto-format and --check to validate if the format is up to date.

@JoeRobich
Copy link
Member

i expected dotnet format to auto-format and --check to validate if the format is up to date.

The --check option does not persist any changes to disk. It is simply for validating that there are no issues reported in the source code.

@OneCyrus
Copy link
Author

yes i know. my problem with this tool is that it reports on issues it isn‘t able to auto-fix.

@OneCyrus
Copy link
Author

i really expected this to behave like it states in the docs.

dotnet format --check Formats but does not save. Returns a non-zero exit code if any files would have been changed.

@JoeRobich
Copy link
Member

I understand your issue better now. This PR #776 changed the behavior of how --check option works with diagnostics and we should have updated the documentation to match.

It loooks like we have two sets of users we need to satisfy. One wants to strictly validate that the options set in .editorconfig are being followed. The other does not want to penalize for issues that cannot be automatically resolved.

@OneCyrus
Copy link
Author

the main problem i see here is that you try to implement dotnet format as a linter and formatter at the same time.

https://taiyr.me/what-is-the-difference-between-code-linters-and-formatters/

imho dotnet format should be a formatter and dotnet build can be used as a linter with ruleset/editorconfig.

@jmarolf
Copy link
Contributor

jmarolf commented Feb 17, 2021 •
edited

While I agree with the overall goal, there exists no mechanism in dotnet build to enforce some of these rules. Someday when that is not the case we can consider leaving enforcement behind and only focus on fixes.

@JoeRobich
Copy link
Member

@OneCyrus Here is a workaround that would work with git. This workflow assumes there are no unstaged changes in the repo. First, you could run dotnet-format without the --check option so that files will be formatted and saved to disk. Then, you would run git diff-files --quiet to determine if there are changes that could be staged.

No files available for staging because no files were fixed during the format.

~/Source/format [git-test]> dotnet format --fix-analyzers info --fix-style info --fix-whitespace -v d
  The dotnet format version is '5.0.211103+b95e1694941ca2595941f1c9cd0d9727b6c53d43'.
  The dotnet runtime version is '6.0.0-alpha.1.21064.11'.
  Formatting code files in workspace '/Users/joeyrobichaud/Source/format/format.sln'.
  Running 52 analyzers on dotnet-format.
  Running 52 analyzers on dotnet-format.UnitTests.
  Running 52 analyzers on dotnet-format.Performance.
  src/Program.cs(44,39): Naming rule violation: These words must begin with upper case characters: run (IDE1006)
  Running 1 analyzers on dotnet-format.
  Unable to fix IDE1006. Code fix NamingStyleCodeFixProvider doesn't support Fix All in Solution.
  Unable to fix IDE1006. Code fix NamingStyleCodeFixProvider doesn't support Fix All in Solution.
  Unable to fix IDE1006. Code fix NamingStyleCodeFixProvider doesn't support Fix All in Solution.
  Running 18 analyzers on dotnet-format.
  Running 55 analyzers on dotnet-format.UnitTests.
  Running 18 analyzers on dotnet-format.Performance.
  Formatted code file '/Users/joeyrobichaud/Source/format/src/Program.cs'.
  Formatted 1 of 97 files.
  Format complete in 8485ms.
~/Source/format [git-test]> git diff-files --quiet
~/Source/format [git-test]> $lastexitcode
0

File available for staging because a file was formatted during the format.

~/Source/format [git-test]> dotnet format --fix-analyzers info --fix-style info --fix-whitespace -v d
  The dotnet format version is '5.0.211103+b95e1694941ca2595941f1c9cd0d9727b6c53d43'.
  The dotnet runtime version is '6.0.0-alpha.1.21064.11'.
  Formatting code files in workspace '/Users/joeyrobichaud/Source/format/format.sln'.
  Running 52 analyzers on dotnet-format.
  Running 52 analyzers on dotnet-format.UnitTests.
  Running 52 analyzers on dotnet-format.Performance.
  Running 18 analyzers on dotnet-format.
  Running 55 analyzers on dotnet-format.UnitTests.
  Running 18 analyzers on dotnet-format.Performance.
  Formatted code file '/Users/joeyrobichaud/Source/format/src/Program.cs'.
  Formatted 1 of 97 files.
  Format complete in 8871ms.
~/Source/format [git-test +0 ~1 -0 !]> git diff-files --quiet
~/Source/format [git-test +0 ~1 -0 !]> $lastexitcode
1

@OneCyrus
Copy link
Author

OneCyrus commented Feb 18, 2021 •
edited

While I agree with the overall goal, there exists no mechanism in dotnet build to enforce some of these rules. Someday when that is not the case we can consider leaving enforcement behind and only focus on fixes.

hmm what's the issue there? we are currently enforcing them with the severity of the rules e.g. for the same issue we can set the rule to error and it breaks the build:

image

@OneCyrus
Copy link
Author

@OneCyrus Here is a workaround that would work with git. This workflow assumes there are no unstaged changes in the repo. First, you could run dotnet-format without the --check option so that files will be formatted and saved to disk. Then, you would run git diff-files --quiet to determine if there are changes that could be staged.

ok. yes that could work.

may be we can get also a new switch like --check-no-file-changed 🙂

@jmarolf
Copy link
Contributor

jmarolf commented Feb 18, 2021

@OneCyrus I assume you are using the brand new to 16.8 <EnforceCodeStyleInBuild>true</EnforceCodeStyleInBuild> option that I added here:
https://github.com/dotnet/sdk/blob/e45057332f15e70831e5db3fdd743ef20a2d6ae5/src/Tasks/Microsoft.NET.Build.Tasks/targets/Microsoft.NET.Sdk.Analyzers.targets#L54 or are you using the experimental codestyle analyzers package?

Either way these are currently experimental, but I have a plan to have it work be fully supported by .NET 6. This option is not available to folks on older versions of the SDK however so We'll need to decide that we want to do there.

@OneCyrus
Copy link
Author

@OneCyrus I assume you are using the brand new to 16.8 <EnforceCodeStyleInBuild>true</EnforceCodeStyleInBuild> option that I added here:
https://github.com/dotnet/sdk/blob/e45057332f15e70831e5db3fdd743ef20a2d6ae5/src/Tasks/Microsoft.NET.Build.Tasks/targets/Microsoft.NET.Sdk.Analyzers.targets#L54 or are you using the experimental codestyle analyzers package?

Either way these are currently experimental, but I have a plan to have it work be fully supported by .NET 6. This option is not available to folks on older versions of the SDK however so We'll need to decide that we want to do there.

no, we don't use this parameter. our builds break when we enforce rules in the .editorconfig file.
but we have also the package SonarAnalyzer.CSharp to enforce additional rules.

@ghost ghost closed this as completed Sep 28, 2021
@ghost
Copy link

ghost commented Sep 28, 2021

Closing this issue as we've seen no reply to the request for more information. If you are able to get the requested information, please add it to the issue and we will retriage it.

@OneCyrus
Copy link
Author

i guess the "needs more info" tag is wrong.

@jmarolf any update on this? especially as you mentioned .NET 6, are there any changes in this regard now that this tool is part of the SDK?

@JoeRobich JoeRobich reopened this Sep 29, 2021
@JoeRobich JoeRobich added Feature Request This issue is requesting an enhancement or new feature and removed Needs More Info This issue needs additional details to be actionable labels Sep 29, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
Feature Request This issue is requesting an enhancement or new feature
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
3 participants
@JoeRobich @OneCyrus @jmarolf