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

Change ParseOutput to return a NonTerminal instead of a Node #1187

Merged
merged 25 commits into from
Jan 13, 2025

Conversation

beta-ziliani
Copy link
Contributor

Solves #1184
To go on top of #1172

This has a big impact in the failing examples, which now return the expected NonTerminal.

This was acknowledgedly coded by playing the whack-a-mole with the type system. I'm not so happy with the bunch of Rc::clone() that leaked out; I will consider an alternative once we settle this is what we need and there are no more pressing issues.

Currently doing a full CI run locally to find what more to fix. In the meantime, I'll mark this as draft.

Copy link

changeset-bot bot commented Dec 11, 2024

🦋 Changeset detected

Latest commit: 6615c18

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 0 packages

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@beta-ziliani beta-ziliani marked this pull request as ready for review December 16, 2024 13:57
@beta-ziliani beta-ziliani requested review from a team as code owners December 16, 2024 13:57
Copy link
Contributor

@OmarTawfik OmarTawfik left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Left a couple of questions. Otherwise, LGTM!
Thanks.

Copy link
Contributor

@OmarTawfik OmarTawfik left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Left some comments above.

Copy link
Contributor

@OmarTawfik OmarTawfik left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated the remaining comments. Thanks!

Copy link
Contributor

@OmarTawfik OmarTawfik left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Left a couple of suggestions. Otherwise, LGTM!

Copy link
Contributor

@OmarTawfik OmarTawfik left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!
I suggest adding a changeset about this.

@beta-ziliani beta-ziliani added this pull request to the merge queue Jan 13, 2025
Merged via the queue into NomicFoundation:main with commit 6389361 Jan 13, 2025
1 check passed
@beta-ziliani beta-ziliani deleted the beta/parse_result branch January 13, 2025 17:10
@beta-ziliani beta-ziliani linked an issue Jan 14, 2025 that may be closed by this pull request
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.

change ParseOutput::Tree to a Nonterminal instead of a Node
2 participants