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

Use ScapegoatConfig.inspections instead of ClassGraph to populate active Scapegoat inspections #132

Merged
merged 3 commits into from Dec 1, 2018

Conversation

@mwz
Copy link
Owner

mwz commented Nov 26, 2018

I didn't realise before that ScapegoatConfig.inspections existed, but this way we can obtain the current list of active inspections, without having to blacklist anything (it looks like we were creating 9 rules which are no longer active in Scapegoat). The use of ClassGraph is no longer necessary in this case.

Closes #131.

Copy link
Contributor

BalmungSan left a comment

👍 cool.

"com.sksamuel.scapegoat.inspections.collections.FilterDotSizeComparison", // Not implemented yet
"com.sksamuel.scapegoat.inspections.collections.ListTail" // Not implemented yet
)
val BlacklistedInspections: Set[String] = Set.empty

This comment has been minimized.

Copy link
@BalmungSan

BalmungSan Nov 26, 2018

Contributor

If it is going to be empty, we should just remove it for now.
If we need it in a future we could just re-implement it, for now it is only noise IMHO.

This comment has been minimized.

Copy link
@mwz

mwz Nov 29, 2018

Author Owner

I wanted to change as little as possible, but yes, I'm happy to remove this.

case clazz if !BlacklistedInspections.contains(clazz.getName) =>
(clazz.getName, clazz.newInstance())
}
ScapegoatConfig.inspections.collect {

This comment has been minimized.

Copy link
@BalmungSan

BalmungSan Nov 26, 2018

Contributor

Following above comment, this can be a simple map.
Also, do we need it to be a List? or are we happy with it being a Seq ?
Or, since you're planning to open a PR in Scapegoat you may change it to List there (that would be better IMHO).

This comment has been minimized.

Copy link
@mwz

mwz Nov 29, 2018

Author Owner

Following above comment, this can be a simple map.

👍

Also, do we need it to be a List? or are we happy with it being a Seq?

I generally tend to go for a Seq as a return type, which is a trait and it makes it easier to use any subtype depending on the needs. I'm not sure why we used a List here, but I'm not too fussed about it since List is a default implementation of Seq and I don't think we have the need to convert this to an indexed sequence anywhere.

This comment has been minimized.

Copy link
@BalmungSan

BalmungSan Nov 30, 2018

Contributor

I used to go for Seq too, but a few days ago I found this and I must admit that he got a point, specially with the complexity.
Anyway, in this case I'm ok with it being a Seq since ScapegoatConfig.inspections is a Seq too, and it will be a List in runtime, and we aren't exposing it.

mwz added 2 commits Nov 29, 2018
@mwz mwz merged commit 379ad6a into master Dec 1, 2018
1 check passed
1 check passed
ci/circleci: test Your tests passed on CircleCI!
Details
@mwz mwz deleted the scapegoat-config-inspections branch Dec 1, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.