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

Improve Parse.Ref #179

Merged
merged 2 commits into from
Sep 20, 2021
Merged

Improve Parse.Ref #179

merged 2 commits into from
Sep 20, 2021

Conversation

marcin-golebiowski
Copy link
Contributor

@marcin-golebiowski marcin-golebiowski commented Sep 19, 2021

It looks like Parse.Ref is having some problems when used heavily together with Or.
Please take a look at my fix, a unit test.

There is also workaround but I would prefer not to use it.
The workaround:

  • Change Parse.Ref(() => parser) to Parse.Ref(() => parser.Named("unique_name"))

Best regards,
Marcin

Unverified

This user has not yet uploaded their public signing key.
@nblumhardt
Copy link
Member

Thanks for the PR, Marcin 👍

Seems like a bug/design limitation of memos, though not having been in the codebase for a while, I can't immediately spot it... happy to push forward with the fix as-is, but if you (or anyone following along) can spot the underlying issue, it'd be great to be able to address it at a lower level 🤔

@marcin-golebiowski
Copy link
Contributor Author

I will try to create additional unit tests later today to check if the fix doesn't break anything.

Unverified

This user has not yet uploaded their public signing key.
…tains info about left recursion
@marcin-golebiowski
Copy link
Contributor Author

@nblumhardt Please find my latest changes.

I think the latest commit contains proper solution to the problem.
I think it's better than my original one.
The initial and second solution solves #166

Best regards,
Marcin

@nblumhardt
Copy link
Member

Awesome! Thank you, Marcin 👍

@nblumhardt nblumhardt merged commit 9d1721b into sprache:develop Sep 20, 2021
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.

None yet

2 participants