Skip to content
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

Add more members to Regex::Options #13223

Conversation

straight-shoota
Copy link
Member

This is a first iteration of the enhancement of Regex::Options to decouple it from any specific libpcre values and also add more defined flags for advanced use cases as described in #13152 (comment)

@beta-ziliani
Copy link
Member

Can't we keep the open option for PCRE? I mean, if -Duse_pcre is specified, we should strive for full backwards compatibility, including extra options.

@straight-shoota
Copy link
Member Author

straight-shoota commented Mar 25, 2023

@beta-ziliani Actually, I'm not sure about that. While we're still supporting automatic fallback to PCRE if PCRE2 is unavailable it seems weird to have different behaviour for the -Duse_pcre flag. I'd prefer that flag to be restricted to purely selecting the library version without other implications and have only a single PCRE behaviour regardless of whether the library was selected automatically or explicitly.
Now we could consider adding an additional flag for the legacy behaviour. But I'm not sure that makes much sense due to very limited use cases.
Instead I'd prefer to focus on covering all relevant use cases in a generic way for both PCRE and PCRE2.

@straight-shoota straight-shoota marked this pull request as ready for review March 26, 2023 13:55
@beta-ziliani
Copy link
Member

can't we simply detect the engine and act upon that?

Copy link
Member

@beta-ziliani beta-ziliani left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think all the issues has been addressed. Unless @Blacksmoke16 or @HertzDevil raise another issue, I consider this ready 🚀

@beta-ziliani beta-ziliani added this to the 1.8.0 milestone Mar 28, 2023
src/regex.cr Show resolved Hide resolved
src/regex.cr Outdated Show resolved Hide resolved
src/regex.cr Outdated Show resolved Hide resolved
@beta-ziliani
Copy link
Member

related specs are failing

@beta-ziliani beta-ziliani removed this from the 1.8.0 milestone Mar 29, 2023
src/regex/pcre.cr Outdated Show resolved Hide resolved
@straight-shoota straight-shoota added this to the 1.8.0 milestone Mar 30, 2023
@straight-shoota straight-shoota merged commit b31b07d into crystal-lang:master Mar 31, 2023
@straight-shoota straight-shoota deleted the feature/regex-options-members branch March 31, 2023 12:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants