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

Add UnnecessaryFilter rule #3341

Merged
merged 15 commits into from
Jan 29, 2021

Conversation

VovaStelmashchuk
Copy link
Contributor

Issue: #3340

@VovaStelmashchuk
Copy link
Contributor Author

I don't have idea about implementation at the moment.

@codecov
Copy link

codecov bot commented Jan 25, 2021

Codecov Report

Merging #3341 (89f248b) into master (731a2db) will decrease coverage by 0.01%.
The diff coverage is 79.41%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master    #3341      +/-   ##
============================================
- Coverage     80.38%   80.37%   -0.02%     
- Complexity     2748     2763      +15     
============================================
  Files           449      450       +1     
  Lines          8300     8335      +35     
  Branches       1587     1599      +12     
============================================
+ Hits           6672     6699      +27     
  Misses          774      774              
- Partials        854      862       +8     
Impacted Files Coverage Δ Complexity Δ
...arturbosch/detekt/rules/style/UnnecessaryFilter.kt 78.78% <78.78%> (ø) 14.00 <14.00> (?)
...rturbosch/detekt/rules/style/StyleGuideProvider.kt 100.00% <100.00%> (ø) 3.00 <0.00> (ø)
...ain/kotlin/io/github/detekt/metrics/LinesOfCode.kt 92.30% <0.00%> (-0.80%) 0.00% <0.00%> (ø%)
...gitlab/arturbosch/detekt/cli/runners/AstPrinter.kt 100.00% <0.00%> (ø) 4.00% <0.00%> (ø%)
...rturbosch/detekt/rules/bugs/UnnecessarySafeCall.kt 75.00% <0.00%> (ø) 5.00% <0.00%> (+1.00%)
...turbosch/detekt/rules/style/UnusedPrivateMember.kt 90.76% <0.00%> (+0.14%) 5.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 731a2db...89f248b. Read the comment docs.

@VovaStelmashchuk
Copy link
Contributor Author

Implementation ready. Can someone review my changes?

@VovaStelmashchuk VovaStelmashchuk changed the title Add tests for UnnecessaryFilter rule Add UnnecessaryFilter rule Jan 25, 2021
…/rules/style/UnnecessaryFilter.kt

Co-authored-by: Chao Zhang <zhangchao6865@gmail.com>
@VovaStelmashchuk
Copy link
Contributor Author

I see codecov report. I can add some test for increase test coverage. But i cannot see reason for add test only for increase test coverage. The additional test will not add real benefits.

@cortinico
Copy link
Member

I see codecov report. I can add some test for increase test coverage. But i cannot see reason for add test only for increase test coverage. The additional test will not add real benefits.

You're fine like that 👍 Codecov coverage report is not a hard requirement for merging. In this case you're at 76.00% of your patch that is totally fine

VovaStelmashchuk and others added 3 commits January 27, 2021 21:09
…/rules/style/UnnecessaryFilter.kt

Co-authored-by: M Schalk <30376729+schalkms@users.noreply.github.com>
@chao2zhang
Copy link
Member

Thank you so much for the contribution and for being patient in the back-and-forth requested changes.

VovaStelmashchuk and others added 2 commits January 28, 2021 22:48
…/rules/style/UnnecessaryFilter.kt

Co-authored-by: Chao Zhang <zhangchao6865@gmail.com>
…/rules/style/UnnecessaryFilterSpec.kt

Co-authored-by: Chao Zhang <zhangchao6865@gmail.com>
@chao2zhang
Copy link
Member

For other maintainers in this PR (@cortinico @BraisGabin @schalkms): We entered 1.16-RC but is it reasonable to add new rules with active = false by default?
I can think of arguments for both sides:

  • We want to only allow bug fixes in RC to speed up the full release
  • We want people to try out experimental new rules early so we can enable them by default faster

@cortinico
Copy link
Member

  • We want to only allow bug fixes in RC to speed up the full release
  • We want people to try out experimental new rules early so we can enable them by default faster

Historically we've been pretty slow at enabling rules by default so I don't mind adding a disabled rule in a RC. Also if we have rules that are ready to go, like this one or #3413, I'll try to squeeze them into 1.16.0 to let them reach our users sooner (so we can get a faster feedback loop).

That being said, I'm in for merging this

@BraisGabin
Copy link
Member

:shipit:! The RC are more to make sure that some changes in the core or clients doesn't break. We did add rules between RC previusly.

@chao2zhang chao2zhang dismissed schalkms’s stale review January 29, 2021 21:44

The review is on an out-dated diff and is addressed later.

@chao2zhang chao2zhang merged commit a260383 into detekt:master Jan 29, 2021
@cortinico cortinico added this to the 1.16.0 milestone Feb 19, 2021
@chao2zhang chao2zhang mentioned this pull request Mar 3, 2021
@VovaStelmashchuk VovaStelmashchuk deleted the add_un_nessesary_filter branch March 5, 2021 08:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants