-
Notifications
You must be signed in to change notification settings - Fork 25
feat: add caller pattern from jinja #422
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
base: master
Are you sure you want to change the base?
Conversation
Signed-off-by: Michael Pollind <[email protected]>
505fec6
to
b468ee3
Compare
Signed-off-by: Michael Pollind <[email protected]>
ced67ce
to
f0cc0cb
Compare
Signed-off-by: Michael Pollind <[email protected]>
7368ac2
to
0c47b3f
Compare
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.
Seems like a very good and solid start! I think the syntax should allow to have default values etc, but can be done in a follow-up. For now though, please support more than just identifiers as arguments when calling it.
Co-authored-by: Guillaume Gomez <[email protected]>
Co-authored-by: Guillaume Gomez <[email protected]>
Signed-off-by: Michael Pollind <[email protected]>
testing/tests/calls.rs
Outdated
source = r#" | ||
{%- macro test() -%} | ||
{{- caller("test") -}} | ||
{{- caller("test") -}} |
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.
Maybe replace "test"
with 1
for this one? Like that we cover integer literals as well. :)
Signed-off-by: Michael Pollind <[email protected]>
probably need a lot more test where to start? |
Hard to say. ^^' I have a few ideas:
And of course, all the same with |
the space handling was wrong didn't understand it on the first pass. I made some more adjustments hopefully the handling is correct now. |
Signed-off-by: Michael Pollind <[email protected]>
1f50bbb
to
a70b7bc
Compare
error: missing `c` argument | ||
--> InvalidNumberArguments.txt:3:18 | ||
"(\"a\", \"b\") -}}\n {%- endmacro -%}\n {%- call(a,b,c) test() -%}\n {{- a"... | ||
--> tests/ui/caller_arguments.rs:5:14 | ||
| | ||
5 | source = r#" | ||
| ______________^ | ||
6 | | {% macro test() %} | ||
7 | | {{- caller("a", "b") -}} | ||
8 | | {%- endmacro -%} | ||
... | | ||
11 | | {%- endcall -%} | ||
12 | | "#, | ||
| |______^ |
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.
this does not look quite correct it should be the call. umm, not sure how to address this.
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.
Strange indeed, seems like the span is created from the start of the arguments and not of caller
.
Signed-off-by: Michael Pollind <[email protected]>
having to wrap every macro in |
f69db26
to
aa1458c
Compare
I added more test cases. and addressed some problems. |
053ce07
to
34764cf
Compare
Signed-off-by: Michael Pollind <[email protected]>
34764cf
to
329aa4e
Compare
It's somewhat supported. Can be revisited though. |
testing/tests/calls.rs
Outdated
)] | ||
struct CallerEmpty {} | ||
let x = CallerEmpty {}; | ||
assert_eq!(x.render().unwrap(), "nested"); |
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.
Can you add the following test to ensure backlines are not removed around "nested" please?
{%- macro test() -%}
{{- caller() -}}
{%- endmacro -%}
{%- call() test() %}
nested
{% endcall -%}
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.
is this expected??
\nnested\n\n
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.
No it's not.
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.
umm i made a mistake somewhere, any clue what I might of done wrong.
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.
Likely. Good Luck with whitespace handling. 😉
If you can't figure it out, I can take a look later on.
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.
yea ... can't quite figure it out
Signed-off-by: Michael Pollind <[email protected]>
3757bf6
to
1a87a45
Compare
Co-authored-by: Guillaume Gomez <[email protected]>
Still need to add tests I mentioned here. |
any clue what this test looks like i'm not quite following
|
Ah sorry, I meant a macro named |
|
Yes, number of arguments should match.
Yep!
Can be added in a follow-up, but then please add an issue and a test which fails on it (with a code comment in the test linking to the issue). |
umm does Expr even supported named arguments? |
Signed-off-by: Michael Pollind <[email protected]>
b25e322
to
f70a3d8
Compare
ref: #282 (comment)
I adjusted this quite heavily given the discussion in issue 282. This both adds the caller macro pattern and changes the syntax to align closer to jinja.
{% call super() %}
is now{{ super() }}
https://jinja.palletsprojects.com/en/stable/templates/#call
https://jinja.palletsprojects.com/en/stable/templates/#child-template