Add additional code comments - #312
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
jgardn3r
left a comment
jgardn3r
left a comment
There was a problem hiding this comment.
Thanks for this! Seems like a pretty good first pass. Makes me want to change the names of some thing XD. This is my first once over, I will need to stew on some of them a little, I think.
| current = next_token_types.pop(); | ||
|
|
||
| // After the current token, some contexts needs to be popped | ||
| // Spring cleaning on contexts that are no longer needed |
There was a problem hiding this comment.
| // Spring cleaning on contexts that are no longer needed | |
| // Remove contexts that are no longer needed |
| /// 1 | ||
| /// {$else} | ||
| /// 2 | ||
| /// {$endif}; |
There was a problem hiding this comment.
I am fairly sure the ; will be on the next line too.
| ParentLineChildComment, | ||
| /// Catch-all for logical lines whose semantics do not inform special formatting. | ||
| Unknown, | ||
| /// "Dead" logical lines that are not to be reformatted or reconstructed. |
There was a problem hiding this comment.
Additionally, this is also about a line being deleted. Since we can't straight up delete the lines, they are assigned to Voided to indicate they've been deleted.
| #[derive(Hash, Debug, Default, Clone, Copy, PartialEq, Eq)] | ||
| pub struct LineParent { | ||
| pub line_index: usize, | ||
| /// Token (as global token stream index) introducing the child line. |
There was a problem hiding this comment.
This is more about what is the parent token of the line. The parent token then is the child line's gateway to being formatted because it is directly dependent on where the parent token gets formatted. Maybe something more like:
| /// Token (as global token stream index) introducing the child line. | |
| /// Token (as global token stream index) whose positioning dictates the formatting of the child line. |
| self.line_type | ||
| } | ||
| /// Exclude this logical line from reconstruction and remove all its tokens. | ||
| /// Returns the removed tokens. |
There was a problem hiding this comment.
I would say something along the lines of "this is effectively deleting the line". As I said in another comment, thats not technically overly possible so this is what we are left with.
| } | ||
|
|
||
| /// Normalized token used by formatters. | ||
| /// Unlike [`RawToken`], the string content is mutable, so formatting passes can adjust token content. |
There was a problem hiding this comment.
Also probably worth noting that it is now TokenType, i.e., all the token types have been finalised to the best of our ability.
| fn ignore_tokens(&self, input: (&[Token], &[LogicalLine]), token_marker: &mut TokenMarker); | ||
| } | ||
|
|
||
| /// Formatting stage that marks certain tokens to be removed during formatting. |
There was a problem hiding this comment.
| /// Formatting stage that marks certain tokens to be removed during formatting. | |
| /// Formatting stage that marks certain tokens to be removed during formatting. |
| /// Index of the current token in the line. | ||
| line_index: u32, | ||
| /// Precedence(0) contexts that should be rewritten to MemberAccess contexts. | ||
| /// This contains all Precedence(0) contexts that have not been marked as [`fluent`]. |
There was a problem hiding this comment.
Is this fluent docref supposed to link somewhere?
2 participants
This PR adds or updates a whole bunch of comments across the source code, focusing particularly on describing what things are so that when you're in the weeds it's easy to keep track of everything.
Feel free to nitpick or tweak as much as you like.