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

fix(pattern): Handle empty alternates #4277

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

loewenheim
Copy link
Contributor

This changes the logic of the pattern parser so that an empty alternate matches the empty string. For example, the pattern "foo{,/}" now matches both "foo" and "foo/" where before it would only match "foo/" (because the empty part before the comma was discarded).

This changes the logic of the pattern parser so
that an empty alternate matches the empty string.
For example, the pattern `"foo{,/}"` now matches
both `"foo"` and `"foo/"` where before it would
only match `"foo/"` (because the empty part before
the comma was discarded.
@loewenheim loewenheim requested a review from a team as a code owner November 21, 2024 15:31
@loewenheim loewenheim self-assigned this Nov 21, 2024
relay-pattern/src/lib.rs Outdated Show resolved Hide resolved
relay-pattern/src/lib.rs Outdated Show resolved Hide resolved
relay-pattern/src/lib.rs Show resolved Hide resolved
Dav1dde
Dav1dde previously approved these changes Nov 22, 2024
relay-pattern/src/lib.rs Show resolved Hide resolved
@Dav1dde
Copy link
Member

Dav1dde commented Nov 22, 2024

As discussed in #4282, instead of smuggling in an empty literal in an alternate, we can instead transform {foo,bar,} into Optional(Alternate(Literal(foo), Literal(bar))) giving us better semantics and easier down-the-line optimizations.

@Dav1dde Dav1dde dismissed their stale review November 22, 2024 10:11

Optional implementation

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