-
Notifications
You must be signed in to change notification settings - Fork 65
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 support for regex quantifiers #121
Conversation
@caleb531 Not quite done yet, but an initial version of the quantifier syntax is working. The main obstacle I'm running into right now has to do with the way that the default input symbols are retrieved. The issue is that to create the factory function for the wildcard, the lexer needs to have all of the non-reserved symbols that appear in a given regex. However, with the default method of retrieving the input symbols, numbers used in the quantifiers will get picked up even though they don't appear as literals in the given regex. Because of the difference between where the factory function is defined and called, this creates a potential sharp edge that may cause issues for some people. Do you have any thoughts on this? There are a couple of ways to get around that but I'm not quite sure which one would be best. |
This reverts commit 275fa65.
@caleb531 looking at this again, how do you feel about changing the default input symbols for creating a regex to something very general, like all alphas + numbers? I think it removes the awkwardness of having the default regex to NFA conversion be only over the symbols in present literals. If not, I think there's a workaround to the wildcard creation issue that keeps this same default, just makes the creation code a little messier. |
@caleb531 going to go ahead and flip this to open now that I had the chance to clean it up. The only controversial thing here is changing the default alphabet used for parsing regex, but I think this actually makes it easier if someone is just playing around with regexes. Performance/memory usage shouldn't be an issue since people can still manually set the input symbols. Haven't updated docs completely, will do once this gets closer to being merged. |
@eliotwrobson Just reviewed this PR again—everything looks good. Did you still have any documentation updates to make before I merge? |
@caleb531 nope! I added the docs updates to the PR already (just noting the quantifier inclusion and changing the default alphabet), so I think this is good to go. If we feel like there's more docs needed, that can be done before this goes on main. |
@eliotwrobson Perfect! Will approve and merge now, then. Thank you for your work on this functionality! |
Pull request for quantifiers, resolves #109