Converted remaining remediators to new API and bugfixes #462
Conversation
andrecsilva
changed the title
There was a problem hiding this comment.
Very cool! We're on the way. Before submitting a PR, make sure the easy stuff (final, public) etc. is right.
I think there is a little more to the architecture to consider here, given the exposure of so many public static methods. Ideally we'd have no static methods that aren't generic utilities. Let's see if we can't iterate and make a solution that doesn't require them?
| import com.github.javaparser.Position; | ||
| import com.github.javaparser.Range; | ||
| import com.github.javaparser.ast.Node; | ||
|
|
There was a problem hiding this comment.
WithoutScopePositionMatcher extends it.
| @@ -16,7 +16,7 @@ public interface FixCandidateSearcher<T> { | |||
| * @param cu | |||
| * @param path | |||
| * @param rule | |||
| * @param issuesForFile | |||
There was a problem hiding this comment.
It is a very cool idea to make this a mutable List. I wrestle with whether is a good idea, because pure functions are easier to test and reason about. I am not inclined to stand in the way, just registering a concern.
There was a problem hiding this comment.
This is admittedly, a bit of a hack since I've failed to accommodate for the problem multiple searchers reporting the same issues as unfixed before working on converting the XXE Remediators, where the problem became very apparent.
I don't think this is that much of a problem wince you shouldn't be using the FixCandidateSearcher outside of SearcherStrategyRemediator now. But I see your point.
I'll think about a solution for this.
framework/codemodder-base/src/main/java/io/codemodder/remediation/NodePositionMatcher.java
Show resolved
Hide resolved
| /** Removes the range of the node's scope before matching it against a position. */ | ||
| public final class WithoutScopePositionMatcher extends DefaultNodePositionMatcher { | ||
|
|
||
| @Override |
There was a problem hiding this comment.
I think this type should be package-private, right? Does it need to be public? If it does, why is getRange not public?
There was a problem hiding this comment.
XXERemediator uses it as parameter. And thus, some codemods (Sonar based ones, mostly) make use of it. Also some tests.
getRange is there mostly to reuse code from DefaultNodePositionMatcher and shouldn't be accessed as it is an implementation detail. I've made it protected.
...a/io/codemodder/remediation/xxe/DocumentBuilderFactoryAndSAXParserAtCreationFixStrategy.java
Show resolved
Hide resolved
...codemodder-base/src/main/java/io/codemodder/remediation/xxe/SAXParserAtNewSPFixStrategy.java
Show resolved
Hide resolved
| * @param node | ||
| * @return | ||
| */ | ||
| public static boolean match(final Node node) { |
There was a problem hiding this comment.
All of these sister types expose this static method -- this is a smell we may need a different architecture?
There was a problem hiding this comment.
Each of those match methods should be their own FixCandidateSearcher, but it gets very verbose.
I'll think on an abstraction to make this easier.
...e/src/main/java/io/codemodder/remediation/xxe/XXEIntermediateXMLStreamReaderFixStrategy.java
Show resolved
Hide resolved
|
No description provided.