Skip to content

Implement forwarding visitors for Option<V> #184

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

Merged
merged 6 commits into from
May 2, 2025

Conversation

apasel422
Copy link
Collaborator

Fixes #183

@apasel422 apasel422 changed the title Implement no-op visitors for Option<V> Implement forwarding visitors for Option<V> Apr 30, 2025
Copy link
Contributor

@martinthomson martinthomson left a comment

Choose a reason for hiding this comment

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

Oh, I like this better than mine, I think. Please take a look at the places where I added Option returns and maybe only implement for those. I don't think that it is necessary to have this for an ItemListVisitor, for example.

@apasel422
Copy link
Collaborator Author

Oh, I like this better than mine, I think. Please take a look at the places where I added Option returns and maybe only implement for those. I don't think that it is necessary to have this for an ItemListVisitor, for example.

IIUC all of the ones I've added so far are necessary. Please take a look and let me know what you think.

@apasel422 apasel422 requested a review from martinthomson May 1, 2025 00:10
Copy link
Contributor

@martinthomson martinthomson left a comment

Choose a reason for hiding this comment

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

Would you mind updating the tests that I wrote to include Some/None as appropriate. I think that you can cherry-pick from #185 for the test changes.

@apasel422 apasel422 marked this pull request as ready for review May 1, 2025 13:13
@apasel422 apasel422 requested a review from martinthomson May 1, 2025 13:22
@apasel422 apasel422 requested a review from valenting May 1, 2025 22:46
Copy link
Collaborator

@valenting valenting left a comment

Choose a reason for hiding this comment

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

This is great, thanks!

Going forward I wonder if we should also try implementing some helpers for the most common uses for parsing. For example, when parsing a dictionary, get the value of an item named x that is expected to be of type string, and (optionally) its parameter y which should be an integer.

@valenting valenting merged commit 907671c into undef1nd:main May 2, 2025
@apasel422 apasel422 deleted the visit-opt branch May 2, 2025 11:11
@apasel422
Copy link
Collaborator Author

Going forward I wonder if we should also try implementing some helpers for the most common uses for parsing. For example, when parsing a dictionary, get the value of an item named x that is expected to be of type string, and (optionally) its parameter y which should be an integer.

I'd be happy to consider that once we get some more feedback, though it would also be worth investigating a serde implementation, at least for deserialization. For now I think we should bump to 0.12.0 and do a release.

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.

Return Option<SomeVisitor> from visitor methods
3 participants