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

builtin/test: refactor the Token enum to be more granular #10357

Merged
merged 6 commits into from Mar 16, 2024

Conversation

The0x539
Copy link
Contributor

Description

This feels somewhat ambitious, but shouldn't actually disrupt any of the "interesting" parts of the parsing or evaluation logic.

  • By separating "unary primary" and "binary primary" tokens into their own sub-enums, the respective matches can be exhaustive with no fallback error case, and the TokenInfo struct, with its flags field, becomes redundant.
  • Further separating the sub-enums makes it easy to factor out the logic associated with each group. The best examples of this are the StatPredicate and NumberComparison cases.
  • The token field on CombiningExpression was already unnecessary, but these changes made that fact visible, because the only time it was accessed was to validate that it's a combiner token (which would be trivially guaranteed now).
  • I also made some minor control flow tweaks in relevant functions to use expression-oriented returns and/or pattern matching. In a few cases, this looks like I rewrote most of the function, but on closer inspection, the code's largely unchanged.

I am not attached to any of the names, and am open to alternatives.

TODOs:

(N/A, no user-facing changes)

Copy link
Contributor

@mqudsi mqudsi left a comment

Choose a reason for hiding this comment

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

I'm not going to bikeshed about the names, but I approve of the general idea. It's slightly less immediately obvious where to add new functionality but I think on the whole less brittle and more maintainable. Good work.

@zanchey zanchey added the rust label Mar 14, 2024
@mqudsi mqudsi merged commit 08220c2 into fish-shell:master Mar 16, 2024
6 of 7 checks passed
@zanchey zanchey added this to the fish next-3.x milestone Mar 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants