rewrite
rewrite copied to clipboard
openrewrite
Enhancement/support glob curly brace
What's changed?
Added support for glob curly braces
What's your motivation?
#3807
Anything in particular you'd like reviewers to focus on?
Anyone you would like to review specifically?
@nmck257 @timtebeek
Have you considered any alternatives or workarounds?
Any additional context
We should be able to change the downstream projects back https://github.com/openrewrite/rewrite-spring/commit/fa4e1c9c20d7373ea1d5d3bf1fdf44a29ab6c537 https://github.com/openrewrite/rewrite-micronaut/commit/c1c0ebd5637b1f145a4bc5f3d4b87d71a4114199
Checklist
- [x] I've added unit tests to cover both positive and negative cases
- [x] I've read and applied the recipe conventions and best practices
- [x] I've used the IntelliJ IDEA auto-formatter on affected files
This looks functionally correct and maintainable to me, which is good! Thanks for picking this up and the other one.
That said, I didn't mention this in the issue, but I'm sensitive to time-performance for the matchesGlob logic. It can become a hot point in things like the Spring Boot upgrade recipes, where lots of Maven recipes lead to lots of groupId/artifactId pattern matches (especially for big poms) which lead to lots of matchesGlob calls.
Prior to #3980, matchesGlob was mostly a low-level, array-crawling implementation which aimed to visit each character of the input+pattern once. Adding in these higher-level checks and loops probably keeps linear performance, but with a higher coefficient, which has me nervous.
I don't have hard numbers, so I can't justify blocking this implementation on my hunch (and am fine if someone wants to approve/merge it). But, if you happen to be interested in experimenting with a crunchier, better-scaling implementation here, I'd enjoy that :)
Thanks for restoring the support for curly braces here @Glanfaloth ! And very thoughtful to link in the downstream changes that we had to do and could revert. Must say I understand @nmck257 's concerns with regards to performance. Did you happen to do any comparison already?
@timtebeek You're welcome! I also agree with Nick, but a better idea hasn't come to my mind so far. I'll keep thinking!
Thanks a lot! I'll mark this as a draft until we've resolved those concerns.
I added a couple more quick offramping checks in the negation and braces cases when those characters don't exist at all in the glob. As for the low-count for loop, I don't see the performance being about the same for that as a Preconditions#or containing several globs, so I think this is only an improvement with little downside.
