Add CSharpier linter

[bdovaz]

https://csharpier.com/

Context: #2173 (comment)

[bdovaz]

@nvuillam do you know why the Docker build fails? In theory I just added a "dotnet tool install -g csharpier"?

I also have another doubt and it is that the configuration file does not have to be passed by command line but it solves it recursively upwards:

https://github.com/belav/csharpier/blob/0.21.0/Src/CSharpier.Cli/ConfigurationFileOptions.cs#L64

How can I do so that it does not include it by command line? I don't know if the field "config_file_name" is used for more things than adding it to the command line and that's why I haven't removed it.

Thanks!

[nvuillam]

Log contains the following

#61 7.338 error NU1202: Package CSharpier 0.21.0 is not compatible with net5.0 (.NETCoreApp,Version=v5.0) / any. Package CSharpier 0.21.0 supports:
#61 7.339 error NU1202:   - net6.0 (.NETCoreApp,Version=v6.0) / any
#61 7.339 error NU1202:   - net7.0 (.NETCoreApp,Version=v7.0) / any
#61 7.406 The tool package could not be restored.
#61 7.406 Tool 'csharpier' failed to install. This failure may have been caused by:
#61 7.406 
#61 7.406 * You are attempting to install a preview release and did not use the --version option to specify the version.

[nvuillam]

It means that #1680 will have to be completed ( upgrade to dotnet v6/v7) before CSharpier can be added

@lextatic any idea about when it could be done ?
@bdovaz maybe can help complete it ? ^^

[bdovaz]

@nvuillam At the moment I can fix it to the latest .net5.0 compatible version (0.16.0): https://www.nuget.org/packages/CSharpier/0.16.0#supportedframeworks-body-tab

Is it ok for you to unlock this PR?

It's just that I see that the .NET 6/7 PR is going to take a while since I see that it has a lot of modifications in the PR.

Note: What I don't know is how to track it later to remove the version fix to 0.16.0.

[nvuillam]

ok for the temp one :)

[bdovaz]

@nvuillam done! Let's wait for the result of the build.

[bdovaz]

@nvuillam the tests fail because it tries to run it with "csharpier" instead of "dotnet csharpier". How do I fix it? Thanks.

[nvuillam]

The command is built here :

megalinter/megalinter/Linter.py

Line 1031 in 133018e

def build_lint_command(self, file=None) -> list:

You have 2 choices:

• define cli_executable: dotnet and cli_lint_extra_args: csharpier
• create a DotnetLinter class that will add dotnet to super.build_lint_command() and add class: DotnetLinter in descriptor ( + same overrides for build_help_command and build_version_command )

If we aim to have more dotnet tools I think it's probably better to create a DotnetLinter class so we can reuse it with future other dotnet linters

[Kurt-von-Laven]

The DotnetLinter class seems like a good plan. dotnet tools are the standard way to package C#/F# tools in my limited experience. We have gotten a lot of mileage out of ReSharper for instance; it is very feature rich and mature.

[bdovaz]

Okay, I got it.

@nvuillam lastly, can you answer me what I asked about the configuration files? #2185 (comment)

Thanks.

[nvuillam]

@bdovaz if you don't want the config file to be added by default if found, you need to remove config_file_name :/

[bdovaz]

@nvuillam But is it safe to do this? I mean, isn't that variable used for anything else? Doing a quick search I see a lot of uses in build.py, config.py and Linter.py.

In case the solution is to remove it then I understand that I also remove it from TEMPLATES/ because it no longer makes sense for it to be there if it's not going to be used.

[nvuillam]

A linter can not have a config file.... but if csharpier can have one, why don't you want to send it as argument ?

Config files in TEMPLATES are the ones by default used when there are none found in the repo

[bdovaz]

A linter can not have a config file.... but if csharpier can have one, why don't you want to send it as argument ?

Config files in TEMPLATES are the ones by default used when there are none found in the repo

@nvuillam It is not that I do not want to send it, it is that I cannot, if you read the message that I have quoted before it comes in the code how it uses it and it is that it solves it in a recursive way upwards without passing it for argument.

https://github.com/belav/csharpier/blob/0.21.0/Src/CSharpier.Cli/ConfigurationFileOptions.cs#L64

If you look at the documentation, it cannot be explicitly passed as an argument:

https://csharpier.com/docs/CLI

[nvuillam]

A linter can not have a config file.... but if csharpier can have one, why don't you want to send it as argument ?
Config files in TEMPLATES are the ones by default used when there are none found in the repo

@nvuillam It is not that I do not want to send it, it is that I cannot, if you read the message that I have quoted before it comes in the code how it uses it and it is that it solves it in a recursive way upwards without passing it for argument.

https://github.com/belav/csharpier/blob/0.21.0/Src/CSharpier.Cli/ConfigurationFileOptions.cs#L64

If you look at the documentation, it cannot be explicitly passed as an argument:

https://csharpier.com/docs/CLI

@bdovaz ok so it means that we can not provide a default config, and can not define argument ^^
But if you define correctly linter_rules_configuration_url it will appear in the doc and users will know in one click how to customize their CSharpier process :)

[Kurt-von-Laven]

That makes sense. CSharpier is inspired by Prettier and similarly aims to be opinionated rather than configurable.

[bdovaz]

@nvuillam fails the test "csharp_csharpier_test::test_failure" and the problem is similar to the other linter I developed and it seems that it never fails and the exit code is always 0 although in reality there are errors.

Steps to reproduce it:

1. Install it:

dotnet tool install --global CSharpier --version 0.16.0

2. Run the following script from ".automation/test/csharp":

@echo off
 
dotnet csharpier --check ./csharp_bad_01.cs

echo Exit code is %errorlevel%.

pause

Output:

What do you think?

[nvuillam]

Current code seems to take that in account, but maybe that with this old version it's not the case :/

[nvuillam]

Maybe you could try a valid file (parsable) but that needs formatting ?

[codecov-commenter]

Codecov Report

Merging #2185 (bcef99f) into main (deb8344) will increase coverage by 0.58%.
The diff coverage is 100.00%.

❗ Current head bcef99f differs from pull request most recent head 08eda82. Consider uploading reports for the commit 08eda82 to get more accurate results

@@            Coverage Diff             @@
##             main    #2185      +/-   ##
==========================================
+ Coverage   82.40%   82.99%   +0.58%     
==========================================
  Files         168      170       +2     
  Lines        4451     4469      +18     
==========================================
+ Hits         3668     3709      +41     
+ Misses        783      760      -23     

Impacted Files	Coverage Δ
megalinter/linters/DotnetLinter.py	100.00% <100.00%> (ø)
...s/test_megalinter/linters/csharp_csharpier_test.py	100.00% <100.00%> (ø)
megalinter/reporters/UpdatedSourcesReporter.py	89.74% <0.00%> (+2.56%)	⬆️
...alinter/tests/test_megalinter/helpers/utilstest.py	89.01% <0.00%> (+8.05%)	⬆️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

[bdovaz]

@nvuillam I almost have it, I got a Trivy error, what do I do, exclude it?

[nvuillam]

It depends, is is avoidable ? If not, is it critical related to the usage of the guilty lib within MegaLinter?

[bdovaz]

@nvuillam what I mean is that it has nothing to do with the changes I have made, it is a similar case to what already happened to me in another PR that for whatever trivy adds more rules at a given time and it starts to affect me in my branch:

[nvuillam]

Yeah but sometimes it's for a good reason, that's why there is so much security about MegaLinter which is itself a security checker :)

About this one, you can add it in .trivyignore

[bdovaz]

@nvuillam Yes, I understand. I was referring to what is the procedure when you are creating a PR and you get a case of this, i.e. a trivy vulnerability in something that you have not touched in this PR. When the solution is to ignore it is easy but if the solution is to fix it it may not be so easy. I was referring to whether the responsibility to fix it belongs to the one who created the PR or to whom.

Once you have clarified this, it may be necessary to write something in the contribution guide.

Finally, the "cli_lint_mode" in this case is file but I see that it also supports to pass directories: https://csharpier.com/docs/CLI

To support this case I have to change to "cli_lint_mode: list_of_files"? I would have to change something else? Example of use:

[Kurt-von-Laven]

Great question. In my opinion, we shouldn't require contributors to fix preexisting issues, so if the same exact error can be reproduced on main, I would say it's not that contributors' responsibility. That being said, sometimes it isn't clear whether an issue is related to a contributors' branch until the issue is understood sufficiently that it may be trivially fixed, but such cases are rare. As we always require a maintainer to sign off before a PR can be merged, I don't see this situation being different in that regard. There have been, and may be in the future, circumstances in which it's pragmatic or necessary to wait to merge a PR that doesn't introduce a build break on account of a preexisting build break. Typically it's still possible to continue work on the PR and address all other issues so that it's ready to go as soon as it becomes possible to merge it. What do you think, @nvuillam?

[nvuillam]

@bdovaz @Kurt-von-Laven usually when there is a new CVE detected by Trivy (or something else breaking CI job even without updates), contributors wait for me to fix the issue in main then they update their branch :)

But issues can also be solved in another PR if it is discussed with the maintainers, like we did here :)

[nvuillam]

And a second linter added by @bdovaz :)

High quality PR, many thanks for your contribution :)

[nvuillam]

@bdovaz plz accept me on twitter & linkedin ^^

[bdovaz]

@nvuillam done!

Can you answer this last question regarding cli_lint_mode?

Finally, the "cli_lint_mode" in this case is file but I see that it also supports to pass directories: https://csharpier.com/docs/CLI

To support this case I have to change to "cli_lint_mode: list_of_files"? I would have to change something else? Example of use:

[nvuillam]

If it can't take a list of files, it's probably better to run it on the whole root folder, using project cli_lint_mode, for better performances (else, calling it file by file sould be awful in big projects)