Allow to pass named arguments to macro calls #910

GuillaumeGomez · 2023-11-17T13:50:00Z

Fixes #885.

Based on https://github.com/djc/askama/pull/909 (so only the last 3 commits are relevant to this PR).

I made a very permissive implementation which allows to mix named and non-named arguments. The only thing we need to check outside of the parser is that the named arguments actually have an equivalent in the called macro. For the rest, everything is checked in the parser directly, which allows nicer error messages.

GuillaumeGomez · 2023-11-20T09:47:03Z

Rebased.

djc · 2023-11-20T10:02:43Z

askama_derive/src/generator.rs

+        {
+            // First we check that all named arguments actually exist in the called item.
+            for arg in args.iter() {
+                if let Expr::NamedArgument(arg_name, _) = arg {


Let's do let .. else for the outer branch here?

I don't really see how it would improve the code since there is no else clause here?

It reduces rightward drift for the further check, so I would still like to do this.

Now there is a good reason for it (the break which was added only recently) so adding it!

askama_derive/src/generator.rs

askama_parser/src/expr.rs

book/src/template_syntax.md

GuillaumeGomez · 2023-11-23T13:10:08Z

Is there anything else to be done here?

askama_parser/src/expr.rs

askama_derive/src/generator.rs

GuillaumeGomez · 2023-11-23T14:51:02Z

Updated! I applied your suggestions and also enforced that named arguments should always be the last arguments. As mentioned above, it allowed to remove a loop since we can check if we have named arguments by checking if the last argument is a named one.

askama_derive/src/generator.rs

askama_parser/src/expr.rs

GuillaumeGomez · 2023-11-24T09:29:32Z

Squashed commits so named arguments coming last are now part of the first commit.

askama_derive/src/generator.rs

askama_parser/src/expr.rs

GuillaumeGomez · 2023-11-24T11:18:36Z

The idea of using a HashMap was really good. I simplified the code quite a lot: I loop through the template arguments, for each of them, I check if there is a named argument with this name in the HashMap and if so use the associated Expr.

With this, we avoid allocating other Vecs and remove a few loops.

I updated the code comments as well.

askama_derive/src/generator.rs

djc · 2023-11-24T12:14:52Z

book/src/template_syntax.md

+{% macro heading(arg1, arg2, arg3, arg4) %}
+{% endmacro %}
+
+{% call heading("something", "b", arg4="ah", arg2="title") %}


Ahh, sorry, these aren't quite the semantics I was going for. The positional arguments should always retain their position, so it should not be allowed to specify arg2 by name here since the call also passes two positional arguments.

That however seems a bit too much of a restriction imo. We can't mix named and unnamed arguments, so I would at least want to be able to specify some arguments by name (because it's not obvious what they are just from the value) whether or not it is one of the first arguments without having to specify all arguments as named ones. If that makes sense to you?

No -- I feel quite strongly that arguments passed as positional should go into the expected positional argument slot, and should not be able to be displaced by later named arguments (IIRC there's precedence for this in how Python does it?).

I disagree but it's your project so I'll update as such.

GuillaumeGomez · 2023-11-24T13:16:05Z

Updated to follow what you asked, added a test to check the compilation error and updated documentation.

djc · 2023-11-28T08:27:57Z

askama_derive/src/generator.rs

+        // * If there is one, we add it and move to the next argument.
+        // * If there isn't one, then we pick the next argument (we can do it without checking
+        //   anything since named arguments are always last).
+        let mut args_iterator = args


The args_iterator approach is unnecessarily complex. We can just have an allow_positional that is initialized to true which is changed to false on the first occurrence of a named argument.

It'll make less good error messages but as you prefer.

How less good? Do you have an example?

Before I could say it's bad to use named argument "x" here. Now I simply provide the argument name of the template macro. I could maybe provide the index too.

But I have to admit, before it was a bit strange too:

is taking the position of an unnamed argument

Not very clear.

GuillaumeGomez · 2023-11-28T09:47:30Z

Updated!

djc

Okay, thanks for sticking with this, I think this is in a pretty good shape now!

GuillaumeGomez · 2023-11-28T10:37:16Z

Thanks for all the review rounds! I'll continue my docs.rs migration and come back here if anything else is missing. :)

djc · 2023-11-28T10:54:23Z

Sounds great!

GuillaumeGomez force-pushed the macro-named-args branch 3 times, most recently from d9aab11 to aa0f1e6 Compare November 20, 2023 09:46

djc reviewed Nov 20, 2023

View reviewed changes

GuillaumeGomez force-pushed the macro-named-args branch 2 times, most recently from c4334fb to 3ac9aa0 Compare November 20, 2023 12:55

djc reviewed Nov 23, 2023

View reviewed changes

GuillaumeGomez force-pushed the macro-named-args branch from 2333266 to 4ce1fd4 Compare November 23, 2023 14:48

djc reviewed Nov 24, 2023

View reviewed changes

askama_derive/src/generator.rs Outdated Show resolved Hide resolved

askama_parser/src/expr.rs Outdated Show resolved Hide resolved

GuillaumeGomez force-pushed the macro-named-args branch from 4ce1fd4 to f197e33 Compare November 24, 2023 09:29

djc reviewed Nov 24, 2023

View reviewed changes

askama_derive/src/generator.rs Outdated Show resolved Hide resolved

askama_derive/src/generator.rs Outdated Show resolved Hide resolved

askama_parser/src/expr.rs Outdated Show resolved Hide resolved

GuillaumeGomez force-pushed the macro-named-args branch 3 times, most recently from 8dbb722 to 3ef34e5 Compare November 24, 2023 11:17

GuillaumeGomez force-pushed the macro-named-args branch from 3ef34e5 to 92b4ac0 Compare November 24, 2023 11:19

GuillaumeGomez commented Nov 24, 2023

View reviewed changes

askama_derive/src/generator.rs Outdated Show resolved Hide resolved

djc reviewed Nov 24, 2023

View reviewed changes

GuillaumeGomez force-pushed the macro-named-args branch 2 times, most recently from 941a6db to 28f3c4d Compare November 24, 2023 13:15

djc reviewed Nov 28, 2023

View reviewed changes

GuillaumeGomez added 3 commits November 28, 2023 10:47

Allow to pass named arguments to macro calls

c47bff4

Add tests for named arguments in macro calls

1cb4c53

Add documentation for macro named arguments

c354990

GuillaumeGomez force-pushed the macro-named-args branch from 28f3c4d to c354990 Compare November 28, 2023 09:47

djc approved these changes Nov 28, 2023

View reviewed changes

djc merged commit 21f840a into rinja-rs:main Nov 28, 2023
20 checks passed

GuillaumeGomez deleted the macro-named-args branch November 28, 2023 10:36

djc mentioned this pull request Dec 5, 2023

Cannot specify single named argument in macro #923

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow to pass named arguments to macro calls #910

Allow to pass named arguments to macro calls #910

GuillaumeGomez commented Nov 17, 2023

GuillaumeGomez commented Nov 20, 2023

djc Nov 20, 2023

GuillaumeGomez Nov 20, 2023

djc Nov 24, 2023

GuillaumeGomez Nov 24, 2023

GuillaumeGomez commented Nov 23, 2023

GuillaumeGomez commented Nov 23, 2023

GuillaumeGomez commented Nov 24, 2023

GuillaumeGomez commented Nov 24, 2023

djc Nov 24, 2023

GuillaumeGomez Nov 24, 2023

djc Nov 24, 2023 •

edited

Loading

GuillaumeGomez Nov 24, 2023

GuillaumeGomez commented Nov 24, 2023

djc Nov 28, 2023

GuillaumeGomez Nov 28, 2023

djc Nov 28, 2023

GuillaumeGomez Nov 28, 2023

GuillaumeGomez commented Nov 28, 2023

djc left a comment

GuillaumeGomez commented Nov 28, 2023

djc commented Nov 28, 2023

Allow to pass named arguments to macro calls #910

Allow to pass named arguments to macro calls #910

Conversation

GuillaumeGomez commented Nov 17, 2023

GuillaumeGomez commented Nov 20, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

GuillaumeGomez commented Nov 23, 2023

GuillaumeGomez commented Nov 23, 2023

GuillaumeGomez commented Nov 24, 2023

GuillaumeGomez commented Nov 24, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

djc Nov 24, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

GuillaumeGomez commented Nov 24, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

GuillaumeGomez commented Nov 28, 2023

djc left a comment

Choose a reason for hiding this comment

GuillaumeGomez commented Nov 28, 2023

djc commented Nov 28, 2023

djc Nov 24, 2023 •

edited

Loading