-
Notifications
You must be signed in to change notification settings - Fork 1
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
feat: add bullet list element parsing and rendering #111
Conversation
peek now also considers matching functions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome!
I think we now have a fairly decent parsing framework. Looks like it's pretty flexible and not that hard to work with, and that makes me really happy.
As for the PR itself, I left some comments. Many of them are not directly related to this PR. You can decide for yourself, whether we should tackle this in this PR.
I am almost leaning towards merging this and then tackling the comments in other PRs so that we prevent explosion in number of changes (is already 1+k LOC large, would not like to inflate it even more). So, I will just leave comments for now (neither approved nor changes requested) so we can discuss first how should we go forward.
fn next(&mut self) -> Option<Self::Item> { | ||
if self.end_reached() { | ||
if self.prefix_mismatch || self.end_reached() { | ||
return None; | ||
} | ||
|
||
if let Some(end_fn) = self.end_match.clone() { | ||
self.next_matching = true; | ||
|
||
if (end_fn)(self) { | ||
self.iter_end = true; | ||
self.next_matching = false; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's somewhat hard for me to follow the code in this whole function in general. Could you add some comments in this function explaining the logic, and/or updating documentation on next_matching
and similar fields to better explain how are they used?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I updated the doc on next_matching
. Hopefully this makes this code clearer.
In short: Setting next_matching
flag in next()
prevents matching in peeking_next()
during execution of the matching functions in next()
. Matching functions must get the symbols as is, because they define what symbols must be skipped.
List of issues that this PR closes
closes #92
Relevant decisions you made in this PR
Removed
Blankline
symbolThe
Blankline
symbol was used to indicate two consecutive newlines.This created problems with prefix matching, because prefix is only matched after newlines.
As a better alternative, the
SymbolIterator
offersis_empty_line()
andconsumed_is_empty_line()
.Those functions also handle lines only consisting of whitespace symbols.
Adapt
peeking_next()
to consider matching functionsWith this change,
peeking_next()
no returns the same symbols asnext()
.This is important for nested elements, because matching functions internally use
peeking_next()
.Meaning that matching functions of nested elements would not be correct if
peeking_next()
would not consider the matching functions of outer iterators.