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.

Sign up for GitHub

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

Jump to bottom

Add String and Char patterns to StringScanner #13806

Merged
merged 2 commits into from Oct 29, 2023
Merged

Add String and Char patterns to StringScanner #13806

merged 2 commits into from Oct 29, 2023

Conversation

funny-falcon
Copy link
Contributor

Ruby since 2.6 added ability to use String as pattern for StringScanner. It is quite usable since it doesn't lead to regexp machinery.

HertzDevil
HertzDevil requested changes Sep 18, 2023
View reviewed changes
src/string_scanner.cr Outdated Show resolved Hide resolved
HertzDevil
HertzDevil requested changes Sep 19, 2023
View reviewed changes
src/string_scanner.cr Outdated Show resolved Hide resolved
src/string_scanner.cr Outdated Show resolved Hide resolved
src/string.cr Outdated Show resolved Hide resolved
@funny-falcon
Copy link
Contributor Author

@HertzDevil , String#byte_index(Char) is in #13819 . After it will be merged, I'll update this PR.

Also I added simpler Rabin-Karp for short needles at #13820

@HertzDevil
Copy link
Contributor

The merge conflicts should be resolved

@funny-falcon
Copy link
Contributor Author

@HertzDevil I've rebased branch.

Additionally, I've deduplicate match method abusing both String and Char have each_byte method.
It could be not fastest way, but it removes need for two methods.

I think, String#starts_with? could accept additional parameter: offset for char offset, and byteoffset for byteoffset. Latter could be used in StringScanner. Python's startswith has start and end options for a while.
But let this happen in other PR.

@HertzDevil
Copy link
Contributor

Please do not rebase. It erases the review history.

@funny-falcon
Copy link
Contributor Author

Please do not rebase. It erases the review history.

I think, Github is smart enough.
Current review remarks were about lines to remove, and lines were removed.

@funny-falcon
Copy link
Contributor Author

@HertzDevil have you remarks or suggestions to current PR commits?

@straight-shoota
Copy link
Member

straight-shoota commented Oct 4, 2023 •
edited

No, seriously: Do not force push.
Please follow the contributor's guide and only add new commits instead of overwriting old ones. This makes the job for reviewers much easier 🙇

@funny-falcon
Copy link
Contributor Author

@straight-shoota , ok, I will not next time. I'm sorry for doing it this time. I doubt I could restore branch in previous state.

@funny-falcon
Copy link
Contributor Author

@HertzDevil , should I fix anything in this PR?

@funny-falcon @straight-shoota
src/string_scanner.cr: use struct for StringMatchData
f9f7f3b 
Co-authored-by: Johannes Müller <straightshoota@gmail.com>
@straight-shoota straight-shoota added this to the 1.11.0 milestone Oct 27, 2023
@straight-shoota straight-shoota changed the title Add ability to use String and Char as pattern in StringScanner Add String and Char patterns to StringScanner Oct 29, 2023
@straight-shoota straight-shoota merged commit 7aa0282 into crystal-lang:master Oct 29, 2023
54 of 55 checks passed
Blacksmoke16 pushed a commit to Blacksmoke16/crystal that referenced this pull request Dec 11, 2023
@funny-falcon @straight-shoota @Blacksmoke16
Add String and Char patterns to StringScanner (crystal-lang#13806)
ab3fef2 
Co-authored-by: Johannes Müller <straightshoota@gmail.com>
@BrewTestBot BrewTestBot mentioned this pull request Jan 8, 2024
Merged
1 task
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@straight-shoota straight-shoota straight-shoota approved these changes

@HertzDevil HertzDevil HertzDevil approved these changes

Assignees
No one assigned
Labels
kind:feature topic:stdlib:text
Projects
None yet
Milestone
1.11.0
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@funny-falcon @HertzDevil @straight-shoota @Blacksmoke16