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

Replace accept_if function with next_if, eat_char and eat_not_char methods #990

Merged
merged 2 commits into from
Feb 13, 2025

Conversation

bjorn3
Copy link
Collaborator

@bjorn3 bjorn3 commented Feb 10, 2025

This makes the call sites slightly clearer and takes advantage of Peekable::next_if instead of manually reimplementing it.

@bjorn3 bjorn3 requested a review from squell February 10, 2025 15:06
@bjorn3
Copy link
Collaborator Author

bjorn3 commented Feb 10, 2025

I noticed that the accept_if function functioned the same as the Peekable::next_if method while reviewing #897

Copy link
Member

@squell squell left a comment

Choose a reason for hiding this comment

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

Since the trait CharStream was removed, that is the case, yes. I do slightly prefer the name accept_if over next_if, but the introduction of eat_char is a terrific improvement for most of the cases accept_if was used (and next_if is a standard function that we can assume familiarity with). So thumbs up for next_if and eat_char.

However, I do not like the eat_not_char function name. I also notice it's only ever used as while eat_not_char('\n'). Since it's unlikely we'll ever use eat_not_char for anything except error recovery/ignoring input, let's just introduce a function ignore_until_eol, skip_to_newline, ignore_rest_of_line, or something similar).

@bjorn3 bjorn3 requested a review from squell February 12, 2025 13:14
…thods

This makes the call sites slightly clearer and takes advantage of
Peekable::next_if instead of manually reimplementing it.
@squell squell merged commit 651cdcf into trifectatechfoundation:main Feb 13, 2025
14 checks passed
@bjorn3 bjorn3 deleted the parser_cleanup branch February 13, 2025 13:54
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