Some small refactorings #26
Conversation
There was a problem hiding this comment.
I'd appreciate if you could change the name of save_if_newline_reached to advance_index with the current implementation. It would make a lot more sense when reading the code with how you reorganized the scan index advancement to be together with the new line handling.
See comments for more context on why this specific change. The gist of it: I expect to advance the index after every scan/action, but right now is not obvious at plain sight.
Other than that loving the changes and learning a lot from it. Thank you very much :). I hope it helped you understand the algorithm better. I'm approving it so you can merge it after doing that name change. Thanks!
| current_syntax_node = current_syntax_node.get_match_node(scan_char(scan_lookahead)) | ||
| until is_finished?(scan_lookahead) | ||
| node = scan_char!(scan_lookahead) | ||
| break unless current_syntax_node.has_match?(node) |
There was a problem hiding this comment.
Nitpick: Using this syntax, how about calling the method has_node_for?(node) or calling the node next_node instead? Is not possible at simple glance to know thatr scan_char brings the mapping to the next node, and not the actual node (which is brought by get_match_node
|
|
||
| save_current_line | ||
| self.current_line = "" | ||
| increment_scan_index!(size) |
There was a problem hiding this comment.
You are coupling the line saving with the progressing the scan index here. I would personally leave the increment_scan_index! where it was (line 68). Although in the current implementation, both go hand by hand (as both are actions that should be taken at the same time), the concepts are together by chance rather than by feature.
Personally I also found it a bit confusing. I guess it should work, but I don't expect the new line storage method to be the one progressing the pointer, but rather the scan/skip methods.
(See next comment) I think it would be mostly solved changing the name of the method to advance_index which is the important thing. The saving new line is a "byproduct" of reaching a newline while advancing the index, plus advancing the index is the main "action" in the parser, and currently is confusing to know when the index is advanced.
|
|
||
| return save_if_newline_reached! if current_action | ||
|
|
||
| looking_for_match_strategy |
There was a problem hiding this comment.
This looks much better like this. Is right, that the special case is just a looking for match strategy (although I think I tried that and didnt woooork...? But I digress, this looks much cleaner if it's working).
Now that I'm seeing this save_if_newline_reached!, another way to put that if you want to do both the index progressing and the newline saving in one method, would be to call it advance_index, and, inside of advance index, leave the method just the same. I think it would be much more readable that way 🤔 just because, I "expect" to look where the index is advanced. Do I make sense?
|
|
||
| return execute_action(action, skipped) if action | ||
|
|
||
| save_if_newline_reached! |
There was a problem hiding this comment.
Same. I think the idea of renaming it to advance_index makes a lot of sense when reading these methods.
This adds some refactorings as I went through the code to try and understand it more.