-
Notifications
You must be signed in to change notification settings - Fork 8
Replace string_split impl with string_split_noregexp #351
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
The string_split function was accidentally supporting regular expressions, which was unintended and conflicted with the design goal of having no dependency on the Java language or library. Changes: - Updated string_split to use SplitNoRegexpProc instead of SplitProc - Removed deprecation warnings from string_split documentation - Updated docs to clarify separator is not a regular expression - Removed string_split_noregexp from experimental/string module as it's no longer needed Fixes ion-fusion#349
toddjonker
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi Chris! Thanks for the contribution.
A couple things need attention:
- We no longer need distinct code paths for regex or not, so we can merge
SplitNoRegexpProcinto itsSplitProcsuperclass. That also preserves the extant Fusion/Java name alignment convention. - The test cases in
ftst/string.test.fusionhave some branching that needs streamlining. I think we should have basically all the current scenarios updated to prove the no-regex behavior. - The relevant test cases in
ftst/experimental/string.test.fusionshould also be pulled from that suite into the previous one. - Be sure to run
./gradlew releaseto make sure the whole test suite passes. It's also a good idea to look at the generate docs (underbuild/docs/fusiondoc) to make sure any changes look as expected.
Thanks again! It'll be great to have this fixed at long last.
| The `separator` is _not_ a regular expression; callers needing that feature | ||
| should use `regexp_split` from the [FusionJavaRegexp][] package, being careful |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This prose needs updating: the reference to FusionJavaRegexp is vestigial from Amazon. But also we don't need to give migration instructions since there's no current users of this public version of the language. (Any use inside Amazon would necessarily need to use an adaptation layer that would need to advise appropriately.)
The relevant regexp_split code is in this repository on the regexp branch, and the plan is to integrate it into the core. That's what we'd want to reference from here, it just doesn't exist yet.
So we should strip out most of this beyond the semantic clarification.
If you're interested in working on #159 (the regexp integration), please comment on that issue (or reach out on Discord) and ask me to sketch out the approach I have in mind.
| "dev.ionfusion.fusion.FusionString$IndexCodePointsProc") | ||
|
|
||
|
|
||
| (defpub_j string_split_noregexp |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did this removal not break test cases?
🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oops, it did: https://github.com/ion-fusion/fusion-java/actions/runs/19654525460/job/56291878967?pr=351#step:5:57-60
You can test locally using the check target.
I recommend running ./gradlew clean release on each PR to ensure full build/test success.
|
Also, welcome! I don't think I've seen you around before, and it's great to see the contribution. Out of curiosity, how did you discover this project? |
|
Hi @cnaples79, are you intended to continue this PR? No problem if not, somebody else will pick it up at some point. Still, I'd love to get this through. If you think the feedback is too onerous to be worth the effort, HMU on Discord and we can seek a middle ground. Thanks again for your effort! |
Summary
string_splitimplementation to useSplitNoRegexpProcinstead ofSplitProcstring_split_noregexpfunction from experimental/string moduleChanges Made
fusion/src/fusion/string.fusion
string_splitto usedev.ionfusion.fusion.FusionString$SplitNoRegexpProcregexp_splitfor users needing regex functionalityfusion/src/fusion/experimental/string.fusion
string_split_noregexpfunction entirely (lines 79-96)string_splitnow has the same behaviorBackground
The
string_splitfunction was accidentally supporting regular expressions, which was unintended and conflicted with the library's design goal of avoiding dependencies on Java language features. Thestring_split_noregexpfunction was introduced as a migration path. Now that enough time has passed, we can complete the migration by replacing the implementation.Fixes #349