Conversation
timtebeek
left a comment
timtebeek
left a comment
There was a problem hiding this comment.
Great to see! Some concern with the approach taken here, as we strive to do no harm. As much as it'll likely mostly be what folks want, we can't be certain they don't rely on the old window-like behavior when applying changes.
| maybeRemoveImport("com.google.common.collect.Collections2"); | ||
| maybeAddImport("java.util.function.Predicate"); | ||
|
|
||
| return JavaTemplate.builder("#{any(java.util.Collection)}.stream().filter(#{any(java.util.function.Predicate)}).toList()") |
There was a problem hiding this comment.
I'm not sure these are as interchangeable ; Collections2.filter returns a com.google.common.collect.Collections2.FilteredCollection which acts as a view on the underlying collection, whereas .toList() returns a fully new collection with very different performance and semantics when adding/removing entries. Any thoughts there?
There was a problem hiding this comment.
You’re certainly right (about the performance), but the current Guava recipes cause compilation errors, as explained in the description of issue #899.
To respect the “do no harm” principle, one option might be to conditionally replace com.google.common.base.Predicate with java.util.function.Predicate.
However, this seems a bit more complex — probably too complex for me at this stage.
I probably thought a bit too quickly when creating my latest issues and PRs, aiming to eliminate all compilation errors and remove any remaining Guava dependencies.
There was a problem hiding this comment.
Yes understandable that you're looking for either a more complete migration off of Guava (with no compilation issues), or a more cautious move off of Guava's Predicate onto Java's Predicate. We have something similar when choosing to move off of more specific Collections methods onto their interfaces, as seen in this recipe:
https://github.com/openrewrite/rewrite-static-analysis/blob/da756464470f9c5dbc056fa2e008123d73a50e05/src/main/java/org/openrewrite/staticanalysis/UseCollectionInterfaces.java#L277-L297
Perhaps good to also/instead add similar handling before we change Predicate: look how it's used, and only make changes when safe to do so.
There was a problem hiding this comment.
Either way appreciate the help both in developing recipes, and thinking through the options. 🙏🏻
2 participants
What's changed?
Collections2.filter(as described in Collections2.filter is not migrated, which creates a conflict with the PreferJavaUtilPredicate recipe #899)Anything in particular you'd like reviewers to focus on?
First contributions, so I’m not sure I’ve mastered all aspects of OpenRewrite yet.
Anyone you would like to review specifically?
@timtebeek
Checklist