Use ScapegoatConfig.inspections instead of ClassGraph to populate active Scapegoat inspections #132
Conversation
| "com.sksamuel.scapegoat.inspections.collections.FilterDotSizeComparison", // Not implemented yet | ||
| "com.sksamuel.scapegoat.inspections.collections.ListTail" // Not implemented yet | ||
| ) | ||
| val BlacklistedInspections: Set[String] = Set.empty |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 { |
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
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 aSeq?
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.
There was a problem hiding this comment.
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.
I didn't realise before that
ScapegoatConfig.inspectionsexisted, 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 ofClassGraphis no longer necessary in this case.Closes #131.