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

Wip/gmt/match find only text #5721

Merged
merged 16 commits into from
Feb 23, 2023
Merged

Wip/gmt/match find only text #5721

merged 16 commits into from
Feb 23, 2023

Conversation

GregoryTravis
Copy link
Contributor

Pull Request Description

Rename is_match + match to match + find (respectively), and remove all non-regexp functionality.

Regexp flags and Match_Mode are also no longer supported by these methods.

Copy link
Member

@jdunkerley jdunkerley left a comment

Choose a reason for hiding this comment

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

Looks good generally a few little changes.

Use method calls not static.
Add return at end of file.
@jdunkerley jdunkerley added the CI: Ready to merge This PR is eligible for automatic merge label Feb 23, 2023
@mergify mergify bot merged commit 3a09ee8 into develop Feb 23, 2023
@mergify mergify bot deleted the wip/gmt/match-find-only-text branch February 23, 2023 09:47
Comment on lines +254 to +257
case_insensitive = case_sensitivity.is_case_insensitive
case Regex.compile pattern case_insensitive=case_insensitive . match self Regex_Mode.All of
Nothing -> []
matches -> matches
Copy link
Member

Choose a reason for hiding this comment

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

If we do the check this way, we lose the locale information from the Case_Sensitivity setting. I understand we cannot easily override the locale setting for Regex - OK. We have the same issue in DB - and I think we should do the same check there too - check if the setting had the default ROOT locale - if so - we can proceed. If the user has overridden the locale to something else, they may count on some custom behaviour that we cannot provide here - so I think in that case we should raise an Illegal_Argument error saying that custom-locale-backed case insensitivity is not supported here.

Copy link
Member

Choose a reason for hiding this comment

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

We are replacing the Regex.compile method in the next ticket and it moves to taking a Case_Sensitive parameter with exactly that logic.

Copy link
Member

Choose a reason for hiding this comment

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

Ah, I did not know this was temporary. Sounds good then!

I think ideally we could add a test case for this too (with this next ticket).

@jdunkerley
Copy link
Member

Closes #5121

@jdunkerley jdunkerley linked an issue Feb 27, 2023 that may be closed by this pull request
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CI: Ready to merge This PR is eligible for automatic merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Update Text.match, Text.find and Text.find_all.
3 participants