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

Refactor CLI args processing #7092

Merged
merged 14 commits into from
Mar 27, 2024
Merged

Refactor CLI args processing #7092

merged 14 commits into from
Mar 27, 2024

Conversation

3flex
Copy link
Member

@3flex 3flex commented Mar 26, 2024

The only functional change is that directories passed to detekt CLI are validated as directories. Other than that, this just makes the code a bit more consistent by using more of JCommander's features, and using stronger typing when args are processed.

@detekt-ci detekt-ci added the cli label Mar 26, 2024
@3flex 3flex added the housekeeping Marker for housekeeping tasks and refactorings label Mar 26, 2024
@3flex 3flex marked this pull request as draft March 26, 2024 11:20
Copy link
Member

@cortinico cortinico left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mostly nits

description = "Input paths to analyze."
)
private var input: String? = null
var inputPath: List<Path> = emptyList()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Q: moving from String? to List<Path> makes an empty input valid? Should we check inside PathValidator that the input is not empty?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If input is empty JCommander returns an error.

java -jar detekt-generator-1.23.6-all.jar -c . -d . -i returns ParameterException: Expected a value after parameter -i.

Any other value is validated to check that the paths exist. The only way to pass an empty list is to pass only a separator like -i , or -i ; but I don't think it's worth guarding against this.

@detekt-ci
Copy link
Collaborator

Warnings
⚠️ This PR is approved with no milestone set. If merged, it won't appear in the detekt release notes.

Generated by 🚫 dangerJS against 578ef51

Copy link

codecov bot commented Mar 27, 2024

Codecov Report

Attention: Patch coverage is 89.65517% with 3 lines in your changes are missing coverage. Please review.

Project coverage is 83.72%. Comparing base (c3ff741) to head (578ef51).
Report is 4 commits behind head on main.

Files Patch % Lines
...itlab/arturbosch/detekt/generator/GeneratorArgs.kt 80.00% 0 Missing and 2 partials ⚠️
...gitlab/arturbosch/detekt/cli/ArgumentConverters.kt 90.00% 0 Missing and 1 partial ⚠️
Additional details and impacted files
@@             Coverage Diff              @@
##               main    #7092      +/-   ##
============================================
+ Coverage     83.67%   83.72%   +0.05%     
+ Complexity     3957     3951       -6     
============================================
  Files           578      578              
  Lines         12164    12147      -17     
  Branches       2514     2502      -12     
============================================
- Hits          10178    10170       -8     
+ Misses          731      730       -1     
+ Partials       1255     1247       -8     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@3flex 3flex merged commit 81bb096 into detekt:main Mar 27, 2024
21 checks passed
@3flex 3flex deleted the generator-jc branch March 27, 2024 05:28
@BraisGabin
Copy link
Member

This PR also have another functional change that I think is good but just to point it out. When passing the inputs we allowed things like this "src/main , foo" and we map that to src/mainandfoo. Now we map it to src/main and foo`.

Also the new exception doesn't allow to spot those errors easily because the path doesn't quote the path so it is really difficult to spot that space. I'm going to create a new PR to fix this last point.

@BraisGabin
Copy link
Member

Also we should use the splitter on the --includes and --excludes too. But I would like to first merge #7007 to avoid more conflicts.

@cortinico cortinico added this to the 2.0.0 milestone Apr 7, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cli housekeeping Marker for housekeeping tasks and refactorings
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants