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

Remove exceptions of Library rules #3304

Merged
merged 3 commits into from
Jan 4, 2021
Merged

Remove exceptions of Library rules #3304

merged 3 commits into from
Jan 4, 2021

Conversation

BraisGabin
Copy link
Member

We had this code to make our 3 library rules behave different. But our users don't understand that #3215.

With this PR I change those rules to behave as any other rule but changing their default configuration so the users can see what's going on.

This is an alternative implementation of #3279

I did disable the rule LibraryCodeMustSpecifyReturnType by default too.

@codecov
Copy link

codecov bot commented Dec 19, 2020 •
edited
Loading

Codecov Report

Merging #3304 (1e83ce9) into master (fb50099) will increase coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff            @@
##             master    #3304   +/-   ##
=========================================
  Coverage     80.32%   80.33%           
+ Complexity     2721     2712    -9     
=========================================
  Files           445      445           
  Lines          8168     8171    +3     
  Branches       1553     1553           
=========================================
+ Hits           6561     6564    +3     
  Misses          774      774           
  Partials        833      833
Impacted Files Coverage Δ Complexity Δ
...sch/detekt/rules/style/ForbiddenPublicDataClass.kt 89.47% <ø> (-0.53%) 11.00 <0.00> (-3.00)
...kt/rules/style/LibraryCodeMustSpecifyReturnType.kt 87.09% <ø> (-0.41%) 9.00 <0.00> (-3.00)
...kt/rules/style/LibraryEntitiesShouldNotBePublic.kt 83.33% <ø> (-0.88%) 7.00 <0.00> (-3.00)
.../detekt/generator/printer/rulesetpage/Exclusion.kt 96.66% <100.00%> (+0.83%) 0.00 <0.00> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update fb50099...1e83ce9. Read the comment docs.

schalkms
schalkms reviewed Dec 27, 2020
View reviewed changes
Copy link
Member

@schalkms schalkms left a comment •
edited
Loading

Choose a reason for hiding this comment

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

I like this idea! The three rules changed their behavior, so this should be documented for the next release. Furthermore, I think all three rules could be activated by default.

...tyle/src/test/kotlin/io/gitlab/arturbosch/detekt/rules/style/ForbiddenPublicDataClassSpec.kt Outdated Show resolved Hide resolved
...tyle/src/test/kotlin/io/gitlab/arturbosch/detekt/rules/style/ForbiddenPublicDataClassSpec.kt Show resolved Hide resolved
detekt-core/src/main/resources/default-detekt-config.yml Outdated
@@ -554,6 +554,7 @@ style:
methods: ['kotlin.io.println', 'kotlin.io.print']
ForbiddenPublicDataClass:
active: false
Copy link
Member

Choose a reason for hiding this comment

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

Maybe we should activate all three rules. With the exclude filter set by default nothing gets flagged, so this is fine.

Copy link
Member Author

Choose a reason for hiding this comment

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

I activated them as you said but I'm not sure about this. This are rules only for library developers. Even if we exclude all the files it seems like rules that should be disabled 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.

Let's see what the community thinks about it.

.../test/kotlin/io/gitlab/arturbosch/detekt/rules/style/LibraryCodeMustSpecifyReturnTypeSpec.kt Show resolved Hide resolved
.../test/kotlin/io/gitlab/arturbosch/detekt/rules/style/LibraryEntitiesShouldNotBePublicTest.kt Show resolved Hide resolved
@schalkms
Copy link
Member

@BraisGabin sorry for my late review. This PR went under my radar, unfortunately.

schalkms
schalkms approved these changes Dec 27, 2020
View reviewed changes
Copy link
Member

@schalkms schalkms left a comment

Choose a reason for hiding this comment

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

All questions are solved. Thanks! PR looks good to me.

@BraisGabin
Copy link
Member Author

Closes #3279

arturbosch
arturbosch approved these changes Jan 4, 2021
View reviewed changes
Copy link
Member

@arturbosch arturbosch left a comment

Choose a reason for hiding this comment

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

Should be clearer for the user, good thoughts!

@BraisGabin BraisGabin merged commit 6b065a8 into detekt:master Jan 4, 2021
@arturbosch arturbosch added this to the 1.16.0 milestone Jan 14, 2021
arturbosch pushed a commit that referenced this pull request Jan 16, 2021
@BraisGabin @arturbosch
Remove exceptions of Library rules (#3304)
88daa7e 
* Fix typo

* exclude all files in our library rules by default

* Active ForbiddenPublicDataClass and LibraryEntitiesShouldNotBePublic by default
@cortinico cortinico mentioned this pull request Nov 7, 2021
Closed
@cortinico cortinico mentioned this pull request Jun 10, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@schalkms schalkms schalkms approved these changes

@arturbosch arturbosch arturbosch approved these changes

@cortinico cortinico Awaiting requested review from cortinico

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
1.16.0
Development

Successfully merging this pull request may close these issues.
3 participants
@BraisGabin @schalkms @arturbosch