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

Deprecate BaseRule and state that it will be make sealed - #2365 #2432

Merged
merged 2 commits into from
Mar 15, 2020

Conversation

arturbosch
Copy link
Member

No description provided.

@arturbosch arturbosch added this to the 1.7.0 milestone Mar 15, 2020
@codecov-io
Copy link

codecov-io commented Mar 15, 2020

Codecov Report

Merging #2432 into master will increase coverage by 0.03%.
The diff coverage is 100%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master    #2432      +/-   ##
============================================
+ Coverage      82.9%   82.93%   +0.03%     
- Complexity     2161     2177      +16     
============================================
  Files           357      359       +2     
  Lines          6148     6206      +58     
  Branches       1122     1133      +11     
============================================
+ Hits           5097     5147      +50     
- Misses          478      480       +2     
- Partials        573      579       +6
Impacted Files Coverage Δ Complexity Δ
...ain/kotlin/io/gitlab/arturbosch/detekt/api/Rule.kt 94.11% <ø> (ø) 13 <0> (ø) ⬇️
.../kotlin/io/gitlab/arturbosch/detekt/api/RuleSet.kt 27.27% <ø> (ø) 1 <0> (ø) ⬇️
.../arturbosch/detekt/cli/runners/SingleRuleRunner.kt 97.22% <ø> (ø) 8 <0> (ø) ⬇️
...otlin/io/gitlab/arturbosch/detekt/core/Detektor.kt 66.07% <ø> (ø) 8 <0> (ø) ⬇️
...otlin/io/gitlab/arturbosch/detekt/api/MultiRule.kt 100% <ø> (ø) 8 <0> (ø) ⬇️
...io/gitlab/arturbosch/detekt/core/rules/RuleSets.kt 100% <ø> (ø) 0 <0> (ø) ⬇️
.../gitlab/arturbosch/detekt/api/internal/BaseRule.kt 100% <100%> (ø) 6 <6> (?)
.../io/gitlab/arturbosch/detekt/cli/Configurations.kt 78.37% <0%> (-0.41%) 0% <0%> (ø)
...sch/detekt/rules/providers/CommentSmellProvider.kt 100% <0%> (ø) 2% <0%> (ø) ⬇️
.../io/gitlab/arturbosch/detekt/cli/runners/Runner.kt 92.3% <0%> (ø) 12% <0%> (ø) ⬇️
... and 9 more

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 8191b95...dfb0e71. Read the comment docs.

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.

We should start to clean up old stuff when v2 of detekt is ready.

@Deprecated("""
Do not use this class directly. Use Rule or MultiRule instead.
This class was introduced to support a common handling of the rule types mentioned.
This class will be made sealed in a different release and you won't be able to derive from it.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
This class will be made sealed in a different release and you won't be able to derive from it.
This class will be made sealed in the next release and you won't be able to derive from it.

@schalkms
Copy link
Member

schalkms commented Mar 15, 2020

Closes #2365 !

…eRule.kt

Co-Authored-By: M Schalk <30376729+schalkms@users.noreply.github.com>
@BraisGabin
Copy link
Member

If you move base rule to an internal package right now you will need to move it outside for 2.0 because all the subclasses of a sealed class should be in the same file.

So I think that it's better to keep baserule in the same place with the depreciation. As soon as we make it sealed it will not be able to be extended so the package will not matter.

@arturbosch
Copy link
Member Author

@BraisGabin @schalkms true that. However if not using a typealias I need to suppress every usage of the BaseRule for our -PwarningsAsError=true strategy ...
And it must be top level so the import is also suppressed.
As we are now deprecating more and more stuff should I go further with the typealias + moving strategy or deactivate warningsAsErrors. What do you think?

@BraisGabin
Copy link
Member

OK, I understand the reason. I'm a bit concern about the binary compatibility but previous changes broke it already so it doesn't matter now. 👍🏻 For me.

@arturbosch arturbosch merged commit 2f78598 into master Mar 15, 2020
@arturbosch arturbosch deleted the deprecate-base-rule branch March 15, 2020 16:18
@arturbosch
Copy link
Member Author

OK, I understand the reason. I'm a bit concern about the binary compatibility but previous changes broke it already so it doesn't matter now. 👍🏻 For me.

Could you elaborate on what is not binary compatible anymore for 1.7?
Imo moving the classes while leaving a typealias behind is binary compatible, isn't it?

@BraisGabin
Copy link
Member

I can be wrong here. I'm not a compatibility expert or nothing near. I think that the typealias are just syntax sugar. But I repeat that I don't know this for sure.

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.

None yet

4 participants