rewrite icon indicating copy to clipboard operation
rewrite copied to clipboard
verified publisher icon openrewrite

Add glob curly-brace support to FindSourceFiles

Open nmck257 opened this issue 2 years ago • 6 comments

Eg {.md,.txt}, like HasSourcePath used to support

Originally mentioned by @timtebeek in https://github.com/openrewrite/rewrite/issues/3755#issuecomment-1838623921

nmck257 avatar Dec 11 '23 16:12 nmck257

Agree it would be best to reinstate those optional selectors. In the move from HasSourcePath to FindSourceFiles we now use PathUtils, which does not support that yet, but is stable across operating systems. https://github.com/openrewrite/rewrite/blob/bbc753423476a5ec557ff70c64593eb28d1c406f/rewrite-core/src/main/java/org/openrewrite/PathUtils.java#L66-L82

timtebeek avatar Dec 11 '23 16:12 timtebeek

@timtebeek - semi-related - would you have any concerns if I widened FindSourceFiles filePattern to @Nullable, and moved logic like this into FindSourceFiles directly? https://github.com/openrewrite/rewrite/blob/b19730693662b69321980e2559a7cab90ccd2512/rewrite-xml/src/main/java/org/openrewrite/xml/RemoveXmlTag.java#L51

At the same time, I'd probably adjust such that empty-string causes that same "match everything" behavior; currently, empty-string seems to match nothing, and afaik, most glob specifications interpret empty-string as "match everything" (and that's also how HasSourcePath worked)

nmck257 avatar Dec 11 '23 22:12 nmck257

Seems reasonable to me, yes thanks!

timtebeek avatar Dec 11 '23 22:12 timtebeek

I'm also noticing that the old HasSourcePath relied on a PathMatcher from the FileSystem, whereas FindSourceFiles uses a homegrown glob implementation in PathUtils.

Do you happen to know if our homegrown impl has desirable qualities (perf, etc) over the "native" impl?

Looks like @shanman190 wrote much of the PathUtils::matchesGlob

nmck257 avatar Dec 12 '23 20:12 nmck257

@nmck257 it's more like fixed up a bit. Haha

The biggest part is that the Java PathMatcher is OS-specific due to being FileSystem specific, so this means that for the OSS build plugins everything works just fine as all LST and recipe execution happens on the immediate host OS, but when you have to have interoperability (such as the Moderne SaaS) between operating systems then it begins to fail without some massaging/assistance.

shanman190 avatar Dec 12 '23 20:12 shanman190

Ah, that's good context. Okay, then adding curly-brace support to our custom PathUtils impl feels like the right choice.

nmck257 avatar Dec 12 '23 20:12 nmck257

Fixed in https://github.com/openrewrite/rewrite/commit/013bd824efbde8525166bd50f567871b2bd18166

timtebeek avatar Mar 03 '24 14:03 timtebeek