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 rule set methods which expose implementation details of detekt-core #2366

Merged
merged 1 commit into from
Feb 29, 2020

Conversation

arturbosch
Copy link
Member

@arturbosch arturbosch commented Feb 23, 2020

Refactorings for #2365 and #2341

@codecov-io
Copy link

codecov-io commented Feb 23, 2020

Codecov Report

Merging #2366 into master will decrease coverage by 0.16%.
The diff coverage is 85.18%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master    #2366      +/-   ##
============================================
- Coverage     82.85%   82.69%   -0.17%     
+ Complexity     2160     2143      -17     
============================================
  Files           352      353       +1     
  Lines          6067     6084      +17     
  Branches       1111     1114       +3     
============================================
+ Hits           5027     5031       +4     
- Misses          469      483      +14     
+ Partials        571      570       -1
Impacted Files Coverage Δ Complexity Δ
...io/gitlab/arturbosch/detekt/api/RuleSetProvider.kt 0% <ø> (-100%) 0 <0> (ø)
.../kotlin/io/gitlab/arturbosch/detekt/api/RuleSet.kt 20% <ø> (-80%) 1 <0> (-7)
...in/kotlin/io/gitlab/arturbosch/detekt/core/Junk.kt 64.7% <0%> (-12.22%) 0 <0> (ø)
.../arturbosch/detekt/cli/runners/SingleRuleRunner.kt 88.88% <100%> (+2.77%) 6 <0> (ø) ⬇️
...otlin/io/gitlab/arturbosch/detekt/core/Detektor.kt 54.05% <100%> (+5.27%) 5 <0> (ø) ⬇️
...io/gitlab/arturbosch/detekt/core/rules/RuleSets.kt 100% <100%> (ø) 0 <0> (?)

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 0bbd0ab...b6f8ecb. Read the comment docs.

Copy link
Member

@BraisGabin BraisGabin left a comment

Choose a reason for hiding this comment

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

I'm trying to understand this refactor. At the end how does we want to RuleSet be like?

just a class like this:

class RuleSet(val id: RuleSetId, val rules: List<BaseRule>, include: String, exclude: String) {
  private val pathFilters: PathFilters = PathFilters.of(include, exclude)

  fun shouldAnalyzeFile(file: KtFile): Boolean = //...
}

And nothing else?

Comment on lines +31 to 32
@Deprecated("Exposes detekt-core implementation details.")
fun accept(file: KtFile, bindingContext: BindingContext = BindingContext.EMPTY): List<Finding> =
Copy link
Member

Choose a reason for hiding this comment

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

Does it? KtFile and BindingContext are from kotlin and Finding is part of our public API.

Copy link
Member Author

Choose a reason for hiding this comment

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

The body of the function exposes how detekt internals work.
The user of the api does not need to know how detekt-core creates instances of the rules and runs over the KtFile's.

Comment on lines +26 to 27
@Deprecated("Exposes detekt-core implementation details.")
fun buildRuleset(config: Config): RuleSet? {
Copy link
Member

Choose a reason for hiding this comment

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

Does it?

Copy link
Member Author

Choose a reason for hiding this comment

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

A rule writer does not need to know how his rules will be created and how the active check is done and how we actually use the includes and excludes to filter if a KtFile should be analyzed. A RuleSetProvider just provide the rules.

@arturbosch
Copy link
Member Author

arturbosch commented Feb 23, 2020

I'm trying to understand this refactor. At the end how does we want to RuleSet be like?

just a class like this:

class RuleSet(val id: RuleSetId, val rules: List<BaseRule>, include: String, exclude: String) {
  private val pathFilters: PathFilters = PathFilters.of(include, exclude)

  fun shouldAnalyzeFile(file: KtFile): Boolean = //...
}

And nothing else?

If I would rewrite detekt now, I would consider this code for a RuleSetProvider:

interface RuleSetProvider {
    val ruleSetId: String

    fun rules(config: Config): List<Rule>
}

A rule author does not need to know that we bundle the rules inside a RuleSet class which has some logic like filtering and isActive etc.
He knows that there are some cli flags and config propeties for this but does not need to see how we iterate over rules, visit them and collect the findings :).

In a 2.0 release I would go further to move the RuleSet to detekt-core and move all the extensions core/rules/RuleSets back to it.
The PathFilters is a good concept but relies on being set on the rule set from detekt-core which I consider a design mistake.

@BraisGabin
Copy link
Member

Oh, OK, so you would remove the rule set from the api all together. That have sense.

@BraisGabin
Copy link
Member

Oh! We should add information in the depreciation message about what the user should use instead. And, if possible, the regex to do it automatically.

@arturbosch
Copy link
Member Author

The problem is the user should not use these methods at all. There is no migration for the user.
He can test his rules one by one. Or if he really wants it he can pull in detekt-test and start a Runner.

@arturbosch arturbosch added this to the 1.7.0 milestone Feb 26, 2020
@arturbosch arturbosch merged commit df894fc into master Feb 29, 2020
@arturbosch arturbosch deleted the 2341-core-refactoring branch February 29, 2020 10:44
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

3 participants