Refactor FuncLexr to use iterator functions #39
Conversation
| None => break, | ||
| while let Some(ch) = self.chars.next() { | ||
| let tok = match ch { | ||
| '{' => self.process_left_brace(), |
There was a problem hiding this comment.
I don't think there is any particular reason we need to make these all functions if they just return a value, i think it muddies the code
There was a problem hiding this comment.
It's true that these functions just return a value.
I did it for consistency and decoupling. If the logic of process_left_brace should change in the future, it's easier for another contributor to say with confidence that the logic is probably contained in the struct method.
Of course it's debatable that inlining the logic is also obvious that the logic is contained there.
I can revert back if you feel strongly about it
crates/vimfuncs/build.rs
Outdated
| if !self.ch().is_whitespace() { | ||
| return; | ||
| } | ||
| fn skip_whitespaces(&mut self) { |
There was a problem hiding this comment.
skip_whitespace is a better name
crates/vimfuncs/build.rs
Outdated
| } | ||
| } | ||
|
|
||
| fn identifier(self) -> String { |
There was a problem hiding this comment.
i don't get why you removed these
There was a problem hiding this comment.
because these functions used on incorrect types (e.g. number on FuncToken::String) will throw an error, and it's not really unreachable
I replaced them with pattern matching the enum because I think it's more idiomatic.
Let me know what you think. I can revert back if needed
|
There's kind of a lot of extra changes in the PR. It's not a huge deal but in general it's nice if the changes can be made smaller so as to be more easily reviewed (for example, some of the name changes and things I don't really like as much as the names that were there before) |
100% agreed. I started making too many changes as part of refactoring. |
|
I cleaned the changes up by
I made a new change so that now lex method iterates through characters by peek() instead of next(). See L64. I think this would make the design more flexible to change in the future. Let me know what you think |
| let ch = self.chars.get(self.position).cloned(); | ||
| self.position += 1; | ||
| fn process_left_brace(&mut self) -> FuncToken { | ||
| self.chars.next(); |
There was a problem hiding this comment.
making it explicit that the iterator advances by one step when creating a token
I am making a minor change to crates/vimfuncs/build.rs.
It doesn't impact the functionality but use more idiomatic iterator. I also made some changes to keep abstraction level consistent. Please let me know if that's too many changes.
Changes:
Testing: