refactor: Minify duplicated Token-to-char conversion with helper - #48
Merged
thatmagicalcat merged 1 commit intoJul 21, 2026
Merged
Conversation
Contributor
Author
|
Unreachable noted in #49 |
There was a problem hiding this comment.
Pull request overview
Refactors the parser’s delimiter handling by centralizing Token → char back-conversion into a single helper on Token, removing duplicated match logic in Parser.
Changes:
- Added
Token::as_char()to convert delimiter tokens (includingEscapevariants) back into their character form. - Updated
Parser::parse_left_delimitedto use the new helper and removed the old duplicated conversion logic (read_delimiter).
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| src/token.rs | Introduces Token::as_char() helper to centralize token-to-character delimiter conversion. |
| src/parser.rs | Refactors parse_left_delimited to use as_char() and deletes duplicated delimiter conversion code. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+432
to
439
| // Peek left and convert it back to it's character representation | ||
| let left = self | ||
| .peek() | ||
| .and_then(|tok| tok.as_char()) | ||
| .ok_or(ParseError::ExpectedDelimiter { side: "left" })?; | ||
|
|
||
| let inner_start = self.pos; | ||
| let mut depth = 0usize; |
Comment on lines
+474
to
478
| // Expect left and right delimiters to match | ||
| // TODO: this should probably get it's own strongly-typed error variant. it's not really | ||
| // unreachable code, and this branch could reasonably be called in production, resulting in | ||
| // a hard panic. | ||
| let expected_right = match left { |
| } | ||
|
|
||
| impl<'a> Token<'a> { | ||
| pub fn as_char(&self) -> Option<char> { |
Owner
|
thanks! I'll merge it for now and hope you'll resolve the unreachable issue |
thatmagicalcat
approved these changes
Jul 21, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The existing parser duplicates the conversion from
Tokenback to corresponding character representation. This commit adds a helper that cleans up excess LOC for the two implementations that existed previously. Saves about nine lines, but more importantly, establishes a single source-of-truth for back-conversion betweenTokenandchar.Ideally, I'd say we write a reversible
From<char>, because it is associative and provides us withInto<char>forToken, but I feel this is a larger architectural choice.Also, as noted with a TODO comment, the unreachable on line 483 isn't really unreachable. This should be addressed, but is outside of the scope of this particular change.