Do not use String equals for Groovy either
#142
Conversation
|
|
||
| // Don't change for Groovy because in Groovy, `==` means structural equality and it's redundant to call equals(). | ||
| @Test | ||
| void doNotChangeForGroovy() { |
There was a problem hiding this comment.
We have a similar separate class to test Kotlin; Was wondering if we shouldn't just collapse these into @Nested test classes in StringLiteralEqualityTest in package org.openrewrite.staticanalysis.
| /** | ||
| * Add a search marker if vising a Groovy file | ||
| */ | ||
| public class GroovyFileChecker<P> extends TreeVisitor<Tree, P> { |
There was a problem hiding this comment.
We could probably make this a bit more generic, so that it can cover even more ground.
@Value
public class IsInstanceOf<P> extends TreeVisitor<Tree, P> {
Class<? extends Tree> treeType;
@Nullable
public Tree visit(@Nullable Tree tree, P p) {
if (tree != null && treeType.isInstance(tree.getClass()) {
return SearchResult.found(tree);
}
return tree;
}
}
Then this may be a candidate for inclusion down in rewrite-core.
There was a problem hiding this comment.
I quite like this suggestion, thanks! Especially eyeing extended support coming up for Groovy, Kotlin, Typescript, Python, ... would help to handle this once in /rewrite and not repeat ourselves per module with such checks.
There was a problem hiding this comment.
@kunli2 Since you're a little more involved with new language development and these exclusions, where would you see this fit best? And would you be willing to take this on?
There was a problem hiding this comment.
Logged as an improvement here, since I'd like to see this fix roll out before we get a response & rework through.
#144
Thanks for the suggestion!
joanvr
left a comment
joanvr
left a comment
There was a problem hiding this comment.
looks good to me. Aside from the more generic approach suggested by @shanman190 , I think it actually fixes the purpose of the PR. So, maybe we can merge this one and iterate over it when we have capacity
What's changed?
Add a
GroovyFileCheckeras precondition toStringLiteralEquality.What's your motivation?
Fixes #140