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

Allow custom & union productions #233

Merged
merged 3 commits into from Jun 13, 2022

Conversation

mccolljr
Copy link
Contributor

This PR will eventually provide an implementation of "custom" productions (a custom parsing function that can parse any arbitrary go value), and "union" productions, which parse a single value from a set of possible values into a container of a single interface type.

The details regarding API and ergonomics will need to be fleshed out further before this can be merged.

context.go Show resolved Hide resolved
grammar.go Show resolved Hide resolved
parser_advanced_test.go Outdated Show resolved Hide resolved
options.go Outdated Show resolved Hide resolved
parser_advanced_test.go Outdated Show resolved Hide resolved
Copy link
Owner

@alecthomas alecthomas left a comment

Choose a reason for hiding this comment

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

Absolutely love it, thanks for doing this!

I have no major objections, it all looks quite reasonable to me. I think the only two issues are lack of documents on the new functions (not unexpected for a draft!), and the name "custom" is not specific enough.

options.go Outdated Show resolved Hide resolved
options.go Outdated Show resolved Hide resolved
options.go Outdated Show resolved Hide resolved
parser_advanced_test.go Outdated Show resolved Hide resolved
parser_advanced_test.go Outdated Show resolved Hide resolved
options.go Outdated Show resolved Hide resolved
ebnf.go Show resolved Hide resolved
parser_advanced_test.go Outdated Show resolved Hide resolved
parser_advanced_test.go Outdated Show resolved Hide resolved
@mccolljr mccolljr marked this pull request as ready for review June 13, 2022 02:15
@mccolljr
Copy link
Contributor Author

Updated naming and documentation, and converted the fat tests to examples. Also added some smaller tests to the main tests suite just to make sure we're covering the new functionality.

@mccolljr mccolljr requested a review from alecthomas June 13, 2022 02:25
Copy link
Owner

@alecthomas alecthomas left a comment

Choose a reason for hiding this comment

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

Again, awesome, thanks for implementing this :)

// In most cases, using MaxLookahead will achieve the same results in practice,
// but with a concrete upper bound to prevent pathological behavior in the parser.
// Using infinite lookahead can be useful for testing, or for parsing especially
// ambiguous grammars. Use at your own risk!
Copy link
Owner

Choose a reason for hiding this comment

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

👍

//
// If the first member matches A, and the second member matches A B,
// and he source string is "AB", then the parser will only match A, and will not
// try to parse the second member at all.
Copy link
Owner

Choose a reason for hiding this comment

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

🙏

@alecthomas alecthomas merged commit 583c334 into alecthomas:master Jun 13, 2022
@mccolljr mccolljr deleted the feat/custom_and_union branch June 13, 2022 12:06
@mccolljr
Copy link
Contributor Author

This is awesome, thanks for coordinating with me to get this merged so quickly!

@alecthomas
Copy link
Owner

Thanks for doing the work :)

PS. I updated the README, and made one minor change: renamed the ParseUnion option to Union, and tagged a new alpha release.

@mccolljr
Copy link
Contributor Author

Thanks for doing the work :)

PS. I updated the README, and made one minor change: renamed the ParseUnion option to Union, and tagged a new alpha release.

Saw that - that's a fine change, naming is hard 😅

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