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

Cli provides list of input files #7007

Merged
merged 2 commits into from Apr 3, 2024
Merged

Conversation

BraisGabin
Copy link
Member

@BraisGabin BraisGabin commented Mar 2, 2024

This is just the first step. Now the cli provides a list of files instead of directories and rules for include/exclude files.

After this change we can do things like #7009 where we can remove a lot of code from :detekt-core and simplify :detekt-tooling

Related with #6866

@BraisGabin BraisGabin requested a review from 3flex March 2, 2024 13:00
@detekt-ci detekt-ci added the cli label Mar 2, 2024
Copy link

codecov bot commented Mar 2, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 83.72%. Comparing base (dbfabdc) to head (6be6482).

Additional details and impacted files
@@            Coverage Diff            @@
##               main    #7007   +/-   ##
=========================================
  Coverage     83.72%   83.72%           
+ Complexity     3952     3950    -2     
=========================================
  Files           578      578           
  Lines         12153    12163   +10     
  Branches       2503     2504    +1     
=========================================
+ Hits          10175    10184    +9     
- Misses          734      735    +1     
  Partials       1244     1244           

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

@BraisGabin BraisGabin mentioned this pull request Mar 2, 2024
@BraisGabin BraisGabin changed the title Cli provide list of files Cli provides list of input files Mar 3, 2024
@BraisGabin BraisGabin added this to the 2.0.0 milestone Mar 8, 2024
Copy link
Member

@3flex 3flex left a comment

Choose a reason for hiding this comment

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

See inline comments - particularly the one for path filtering.

@BraisGabin
Copy link
Member Author

I broke something... I need to check.

@@ -3,7 +3,7 @@
-b
./config/detekt/baseline.xml
-ex
**/resources/**,**/detekt*/build/**,**/build-logic/build/**,**/build-logic/bin/**
**/resources/**,detekt*/build/**,build-logic/build/**,build-logic/bin/**
Copy link
Member Author

Choose a reason for hiding this comment

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

Now that we don't take into account the absolute path when you provide excludes/includes you need to be more careful with the initial **/. **/ match at least one directory. If your directory is on the basePath then it doesn't match. For that reason I needed to update these excludes.

Copy link
Member

Choose a reason for hiding this comment

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

If your directory on the basePath then it doesn't match

This will need to be called out loud & clear in the release notes - I also don't recall if basepath is always set but we should make sure it's always set to a reasonable default value by default.

Copy link
Member

Choose a reason for hiding this comment

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

(it's the right thing to do though!)

Copy link
Member Author

Choose a reason for hiding this comment

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

About default values for bashPath:

  • cli uses the working directory.
  • gradlePlugin as far as I saw it passes the project path.

Both of them seems like good default values to me.


And about call out this change, I agree. This is a breaking change on the cli. But I'm not sure what can I do now about it.

@BraisGabin BraisGabin marked this pull request as draft March 25, 2024 09:59
@BraisGabin BraisGabin marked this pull request as ready for review March 25, 2024 10:13
@BraisGabin BraisGabin added the breaking change Marker for breaking changes which should be highlighted in the changelog label Mar 25, 2024
@BraisGabin BraisGabin changed the base branch from main to improve-cli-tests March 27, 2024 11:33
Base automatically changed from improve-cli-tests to main March 27, 2024 22:18
@BraisGabin BraisGabin merged commit 3cd9181 into main Apr 3, 2024
21 checks passed
@BraisGabin BraisGabin deleted the cli-provide-list-of-files branch April 3, 2024 14:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking change Marker for breaking changes which should be highlighted in the changelog cli
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants