Implement is (not) defined feature
#78
Conversation
|
Sorry, but I won't find the time to review before Friday/Saturday. |
rinja_parser/src/expr.rs
Outdated
| let (i, left) = Self::filtered(i, level)?; | ||
| // This is cheating: to make it easier, we expect `defined` to be provided afterward | ||
| // in order to keep the same code logic as the other calls. | ||
| let (i, right) = many0(pair(tuple((ws(tag("is ")), ws(opt(tag("not "))))), |i| { |
There was a problem hiding this comment.
let (i, rhs) = opt(preceded(
ws(keyword("is")),
opt(terminated(
opt(keyword("not")),
ws(keyword("defined")),
)),
))(i)?;
let is_neg = match rhs {
None => return Ok((i, lhs)),
Some(None) => return Err("expected: (not) defined"),
Some(Some(None)) => false,
Some(Some(Some(_))) => true,
};
You say that and then provide suggestion right away. 😆 But in any case, it's not in a hurry. So please review when you have time and motivation to keep this project fun for you. :) |
GuillaumeGomez
force-pushed
the
is-defined
branch
from
55d4bd9
to
4167459
Compare
|
Finally applied your suggestion for the parser. Looks much better like this, thanks! |
Thinking some more about this, I think we can simply create a new very small AST for conditions which would simply remove the conditions (and their associated block) that won't be rendered and then the function can simply loop through them instead of the whole state machine. Gonna do that. |
|
Moved the logic out of the |
Thank you, much easier to read! I give you a proper review soon-ish. |
rinja_parser/src/expr.rs
Outdated
| expr_prec_layer!(muldivmod, is_defined, alt((tag("*"), tag("/"), tag("%")))); | ||
|
|
||
| fn is_defined(i: &'a str, level: Level) -> ParseResult<'a, WithSpan<'a, Self>> { | ||
| let (_, level) = level.nest(i)?; |
There was a problem hiding this comment.
I think we can to without the nesting check in here. We only traverse to a lower level, not the same or a higher level.
There was a problem hiding this comment.
Don't we need that for Self::filtered(i, level)? just below?
There was a problem hiding this comment.
No, you can just pass the level as is. In all other cases we also only increment the recursion level on recursions. The code cannot parse an expression like y is defined is defined, so no nesting happens in here.
There was a problem hiding this comment.
Good point! Removed this nesting.
rinja_parser/src/expr.rs
Outdated
| Some(Some(Some(_))) => true, | ||
| }; | ||
| match *lhs { | ||
| Self::Var(_) => {} |
There was a problem hiding this comment.
Please capture the variable name in here.
rinja_parser/src/expr.rs
Outdated
| i, | ||
| WithSpan::new( | ||
| if is_neg { | ||
| Self::IsNotDefined(Box::new(lhs)) |
There was a problem hiding this comment.
The use the variable name in here. No need for boxing. This makes the code a lot more simple, I think.
There was a problem hiding this comment.
That was an incredible idea! Code got simplified by quite a lot! :D
|
Sorry for the iterative reviews! It's a bit hard for me to wrap my head around this feature. |
GuillaumeGomez
force-pushed
the
is-defined
branch
2 times, most recently
from
39e9f21
to
edf870f
Compare
It's fine. Don't hesitate if you need some clarifications or any help to understand the code (which, if not easy to understand, should be more documented! So don't hesitate to tell me what's unclear so I can add code comments 😃) |
|
I pushed a commit to remove the line of unsafe code. I actually wanted to look if it was easily possible to remove it, and the best test was implementing it. :D The code looks good, and after reading your comments properly, I understand it perfectly (I think). Great work! |
GuillaumeGomez
force-pushed
the
is-defined
branch
from
0bcba4f
to
b974f67
Compare
|
Fixed the merge conflict and applied your suggestion. :)
That's my first implementation. I removed it to reduce the number of lifetimes. If you prefer with references, then I'm fine with it. :)
Awesome! \o/ |
GuillaumeGomez
force-pushed
the
is-defined
branch
from
bedfe55
to
5c5d9a2
Compare
|
Reverted wrong nested ui tests changes. |
|
I'll let you merge (still need your approval to unlock merge buttons 😉 ). |
Fixes #62.
I'm not super happy about how I implemented the parser part (the little "cheat") but found it easier to be done this way to keep the same logic as the other operators. But it forced to have some more error checking.
The other downside is that it makes the code to generate the
if/elsebranches a bit messy. If you have ideas on how to make it look better by any chance?