Improve csharpier performance by changing cli_lint_mode to list_of_files

[bdovaz]

No description provided.

[codecov-commenter]

Codecov Report

Merging #2198 (54cc29e) into main (9019e8a) will increase coverage by 0.02%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##             main    #2198      +/-   ##
==========================================
+ Coverage   82.97%   82.99%   +0.02%     
==========================================
  Files         170      170              
  Lines        4469     4469              
==========================================
+ Hits         3708     3709       +1     
+ Misses        761      760       -1     

Impacted Files	Coverage Δ
megalinter/reporters/UpdatedSourcesReporter.py	89.74% <0.00%> (+2.56%)	⬆️

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

[nvuillam]

u can ignoe the trivy error ^^

[nvuillam]

Trivy fixed in #2200

[echoix]

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)

Originally posted by @nvuillam in #2185 (comment)

The documentation is a little vague, but it seemed to suggest that it can be passed multiple files. So, I just made a quick test with the Dapper codebase.

dotnet csharpier . --check
dotnet-csharpier --check .

dotnet csharpier --check "./Dapper/CommandFlags.cs" "./Dapper/FeatureSupport.cs"

dotnet-csharpier --check "./Dapper/CommandFlags.cs" "./Dapper/FeatureSupport.cs"

All of these work fine. I added the --check option for it to not change any files, and limited myself to two files, that had changes that would've been fixed.

So @bdovaz, you are free to use csharpier in any cli_lint_mode, (or implement them all so a user could tune its config).
I would be curious of a real world test on a bigger C# codebase, to see if our performance on listing and filtering the files on the Python side really is slower than what a compiled C# tool can achieve.

Also, in the example I wrote I showed how to use the dotnet-csharpier as I think there was some kind of argument difficulties when calling it a couple days ago. Creating and maintaining a class might not have been necessary after all. When a dotnet tool is installed globally, and the tool name is prefixed with dotnet-, then we can call it without the dotnet- prefix with dotnet ((the tool name)). See https://learn.microsoft.com/en-us/dotnet/core/tools/global-tools, especially https://learn.microsoft.com/en-us/dotnet/core/tools/global-tools#invoke-a-global-tool.

[bdovaz]

@nvuillam @echoix to make it clear to me:

1. In the case that I want to support the 3 modes: (file, list_of_files and project) what would I have to put in "cli_lint_mode"? Because I understand that this value is used later to generate the documentation in markdown. In the case of JavaPmd I see that list_of_files is set although it supports the 3 modes:

https://github.com/oxsecurity/megalinter/blob/v6.17.0/megalinter/descriptors/java.megalinter-descriptor.yml#L76

In fact my doubt arises, "cli_lint_mode" should not be an array and add the modes supported (apart from another "cli_lint_mode_default" or similar)?

cli_lint_mode:
  - file
  - list_of_files
  - project
cli_lint_mode_default: list_of_files

Because otherwise how can someone who wants to use a particular linter know? In this example you can see that it names the 3 but it doesn't mean that you can use all 3, no? If you put one that is not supported by that linter, what happens? Is there any logic to check this?

https://megalinter.io/latest/descriptors/java_pmd/

3. I should remove this:

cli_lint_extra_args_after:
  - "."

And in a new CsharpierLinter.py do something like this, no?

https://github.com/oxsecurity/megalinter/blob/v6.17.0/megalinter/linters/JavaPmdLinter.py#L38

Note: depending on how some of the questions in my message evolve, I will create other issues or PRs.

[nvuillam]

You can use cli_lint_mode: list_of_files, if it works like that no need to invest time on project mode ( if we wanted to do so, we'd probably have to increment something like cli_lint_extract_args_project to add the . argument only in project mode

[bdovaz]

@nvuillam but if I put it as "list_of_files" as you say it will no longer work in "project" mode. In fact, I have to remove "cli_lint_extra_args_after" because otherwise it concatenates a "." (dotnet csharpier --check myfile.cs .) at the end and it's something I only want in "project" mode.

What we want with this PR is to support all 3 modes, no? Since it is possible through the CLI it makes sense to implement it, no?

About generated docs improvements I have created an issue so that it will not be forgotten: #2202

[nvuillam]

@nvuillam but if I put it as "list_of_files" as you say it will no longer work in "project" mode. In fact, I have to remove "cli_lint_extra_args_after" because otherwise it concatenates a "." (dotnet csharpier --check myfile.cs .) at the end and it's something I only want in "project" mode.

What we want with this PR is to support all 3 modes, no? Since it is possible through the CLI it makes sense to implement it, no?

About generated docs improvements I have created an issue so that it will not be forgotten: #2202

if list_of_files works, file implicitely works too :)
If default is list of files, and that someone wants to override to project mode, then they will need to add a config var
CSHARP_CSHARPIER_ARGUMENTS: ['.'] , i'm ok with that as the most performant mode is managed by default :)

[bdovaz]

if list_of_files works, file implicitely works too :) If default is list of files, and that someone wants to override to project mode, then they will need to add a config var CSHARP_CSHARPIER_ARGUMENTS: ['.'] , i'm ok with that as the most performant mode is managed by default :)

@nvuillam But in that case isn't it better that I create a class (like JavaPmdLinter) and handle that internally? It's weird that someone has to know that he has to add an extra argument (linter specific), isn't it? It forces you to have to look at the specific documentation for that linter. By doing what I say, you could directly change the variable "CSHARP_CSHARPIER_CLI_LINT_MODE: project" and it would work without doing anything else.

[nvuillam]

If list_of_files works well, there is usually no reason to switch to project mode :)

[bdovaz]

@nvuillam ready for you to review and merge!

[nvuillam]

@bdovaz plz can you add a good_2 and a bad_2 test file ?
list_of_files mode linters have to be tested with multiple files :)

[nvuillam]

Plz check comment :)

[bdovaz]

Plz check comment :)

@nvuillam I have already added a second file to each test, is there anything left?

[nvuillam]

my post was before you added the 2 files , if CI is ok you're all good :)

[nvuillam]

Great PR as usual :)