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

Refined some basic concepts of the crate #2

Open
wants to merge 1 commit into
base: develop
Choose a base branch
from

Conversation

alexpyattaev
Copy link

Hi! I've started checking the code for memory soundness around the Box::leak idea you've used, and, as one would expect, it was leaking memory. So I ended up changing quite a bit of logic around the idea of how Text gets formatted and escaped to avoid memory leaks without breaking the convenience of using &str for chunks of the regex being built. I think the current version is far better in terms of its internal logic and it avoids making RE engine to escape things in the Text portions of the regex. But then I got carried away and changed other things as well... Hope you like them!

Another change is around Exactly, it was kind of misnamed, as it was matching whole words, not whole lines. Furthermore, the pattern of using Exactly(Word) was silly, so I moved things around a bit to make it possible to write something like

Times(Digit, 4).grouped_as("year")
.and(Text("-"))
.and(Times(Digit, 2).grouped_as("month"))
.and(Text(("-")))
.and(Times(Digit, 2).grouped_as("day"));

which feels far less cluttered then the original version with Exactly wrapping every Text tag.

Finally, I've moved grouping into its own trait and implemented it for Type as well as Input objects. This makes it possible to define groups over things like single Digit objects, which is quite nice to use.

Overall, I hope you like these changes, looking forward for your feedback.

@Heiss
Copy link
Owner

Heiss commented Jun 1, 2024

A wow! Thanks for your contribution. I will need some time to go through your changes.

@alexpyattaev
Copy link
Author

alexpyattaev commented Jun 1, 2024 via email

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants