Add suppress_whitespace config option and add `config` parameter to the derive macro #664

GuillaumeGomez · 2022-04-07T16:53:19Z

I recently disovered in jinja (with Flask in python) that we could enable automatic_block_trim, allowing us to remove all the - and also preventing us to forget them. I saw it was missing in this crate so I thought it'd be a good addition.

So now, about the newly added config parameter in the derive macro: I realized when writing the tests that I couldn't have a specific configuration for a given test, making it quite complex. So the easiest solution was to create this new parameter. If you prefer, I can open a separate PR for it (its implementation is in the second commit). Of course, it's only if both features make sense to you.

Finally: I didn't find out where to put the documentation for config. Do you have a suggestion?

In any case: thanks a lot for writing this crate! It's really nice to use.

askama_shared/src/generator.rs

testing/tests/gen_ws_tests.py

GuillaumeGomez · 2022-04-08T12:25:23Z

Fixed the typos. :)

djc

Did you see the earlier discussion in #598? In addition to the smaller questions below, I'm not sure I like the specified behavior much where the only effect of a - when automatic_block_trim is enabled is to disable the trim on the other side of the block. IMO the design I think we discussed earlier where a + could be added to locally revert the default seems much clearer, since it is explicit and does not cause sort spooky action at a distance (even if the distance is limited).

What do you think? I forget what Jinja and Tera do exactly...

askama_shared/src/generator.rs

book/src/configuration.md

GuillaumeGomez · 2022-04-11T09:57:33Z

Did you see the earlier discussion in #598? In addition to the smaller questions below, I'm not sure I like the specified behavior much where the only effect of a - when automatic_block_trim is enabled is to disable the trim on the other side of the block. IMO the design I think we discussed earlier where a + could be added to locally revert the default seems much clearer, since it is explicit and does not cause sort spooky action at a distance (even if the distance is limited).

What do you think? I forget what Jinja and Tera do exactly...

I just checked and they indeed use + to revert the automatic trimming (my bad, I thought the - was doing it). So if you agree, I can make the change.

Btw, what do you think of the new derive template argument?

djc · 2022-04-11T09:59:24Z

I think adding a config argument to the template attribute makes sense in principle, will review the changes in more detail once you've split the commit.

GuillaumeGomez · 2022-04-11T10:03:11Z

Thanks for confirming. Then I'll make the changes as soon as possible.

GuillaumeGomez · 2022-04-11T19:03:21Z

Updated! Little sum up of what I did:

I split the refactoring into its own commit.
I then made a new commit where I add the support for the new config parameter.
I adding the parsing for the + into its own commit as well.
I extended the tests to handle both + and - (in case the automatic_block_trim option is enabled).
I extended the documentation for automatic_block_trim.
I added documentation for config (finally found where the template parameters documentation was).

djc · 2022-04-11T19:12:30Z

Sounds great! I'll review this tomorrow.

askama_shared/src/generator.rs

askama_shared/src/parser.rs

askama_shared/src/lib.rs

testing/tests/gen_ws_tests.py

book/src/configuration.md

GuillaumeGomez · 2022-04-13T12:35:53Z

Applied suggestions!

askama_shared/src/generator.rs

GuillaumeGomez · 2022-04-13T13:00:57Z

Fixed the invalid rebase. Sorry for the noise.

djc

This looks great, thanks!

djc · 2022-04-13T13:08:48Z

@Kijewski @vallentin any opinions?

GuillaumeGomez · 2022-04-18T10:44:33Z

Any news here? :)

djc · 2022-04-18T19:20:07Z

Given that it's been the Easter weekend, I'll give them like 24 more hours to respond. :)

GuillaumeGomez · 2022-04-18T19:21:53Z

That is a very good point. We'll see if they survived the chocolate day. 😆

vallentin

I really like the direction this is heading. Unless I'm misinterpreting something, and please correct me if I am. For HTML I'd love if it was possible to say "Trim all but 1 space". Given that in HTML FooBar vs Foo Bar are rendered differently.

While Foo Bar, Foo Bar, Foo\n\n<Bar, Foo\n\n \n \n\n\nBar are all rendered the same.

Anyways, this is more of a side-comment, but a feature I'd love to see extended later :)

askama_shared/src/generator.rs

GuillaumeGomez · 2022-04-19T09:02:29Z

I really like the direction this is heading. Unless I'm misinterpreting something, and please correct me if I am. For HTML I'd love if it was possible to say "Trim all but 1 space". Given that in HTML FooBar vs Foo Bar are rendered differently.

While Foo Bar, Foo Bar, Foo\n\n<Bar, Foo\n\n \n \n\n\nBar are all rendered the same.

Anyways, this is more of a side-comment, but a feature I'd love to see extended later :)

It only impacts jinja block, not pure HTML. So unless you have a jinja block between your two , it won't change the whitespace there (I hope you understood correctly ^^').

vallentin · 2022-04-19T09:43:27Z

It only impacts jinja block, not pure HTML. So unless you have a jinja block between your two , it won't change the whitespace there (I hope you understood correctly ^^').

Yeah, I interpreted that much. But as in example, I have some loops like <ul> {% for %} <li>...</li> {% endfor %} </ul>, then the space between the <li> elements would be removed, right? :)

GuillaumeGomez · 2022-04-19T09:45:04Z

Absolutely! But if you want to keep them, you can use + like this: <ul> {% for %+} <li>...</li> {+% endfor %} </ul>.

vallentin · 2022-04-19T11:53:23Z

Absolutely! But if you want to keep them, you can use + like this: <ul> {% for %+} <li>...</li> {+% endfor %} </ul>.

Yeah, of course, and it's really nice, don't get me wrong :)

My point was "a feature I'd love to see extended later". I'd love to see an extension of this feature in the future, where it would be possible to configure, so it trims all but 1 space between all "items" :)

GuillaumeGomez · 2022-04-19T11:57:29Z

Oh I see. Sorry I misunderstood your point. Yes, it would be a nice extension! Don't forget to open an issue so it's not forgotten. 😉

Kijewski

I like the idea, and the implementation looks good to me. Nice!

GuillaumeGomez · 2022-04-21T08:37:39Z

Thanks a lot everyone for the careful reviews and suggestions, it was very nice contributing here! I'll try to take a look at @vallentin's suggestion too.

djc · 2022-04-21T08:41:33Z

@GuillaumeGomez thanks to you for doing the work on this change and dealing with my feedback!

vallentin reviewed Apr 8, 2022

View reviewed changes

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

testing/tests/gen_ws_tests.py Outdated Show resolved Hide resolved

testing/tests/gen_ws_tests.py Outdated Show resolved Hide resolved

GuillaumeGomez force-pushed the automatic-trim branch from 4efa301 to 3b4b4c8 Compare April 8, 2022 12:25

vallentin requested a review from djc April 8, 2022 13:53

djc reviewed Apr 11, 2022

View reviewed changes

askama_shared/src/generator.rs Show resolved Hide resolved

book/src/configuration.md Outdated Show resolved Hide resolved

GuillaumeGomez force-pushed the automatic-trim branch from 3b4b4c8 to ec1f380 Compare April 11, 2022 19:00

djc reviewed Apr 13, 2022

View reviewed changes

GuillaumeGomez force-pushed the automatic-trim branch from ec1f380 to cf8dcc1 Compare April 13, 2022 12:35

GuillaumeGomez changed the title ~~Add automatic_block_trim config option and add config parameter to the derive macro~~ Add suppress_whitespace config option and add config parameter to the derive macro Apr 13, 2022

djc reviewed Apr 13, 2022

View reviewed changes

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

GuillaumeGomez added 7 commits April 13, 2022 14:56

Add suppress_whitespace config option

526135f

Update parser to allow "+" sign

f14129e

Parse template derive args outside of TemplateInput::new

921403d

Add config option to derive macro so we can specify config file location

7d4c4fa

Add test for suppress_whitespace option

a57a89d

Add documentation for suppress_whitespace

8e523cb

Add documentation for "config" parameter

5f7dcaa

GuillaumeGomez force-pushed the automatic-trim branch from cf8dcc1 to 5f7dcaa Compare April 13, 2022 13:00

djc approved these changes Apr 13, 2022

View reviewed changes

vallentin reviewed Apr 19, 2022

View reviewed changes

askama_shared/src/generator.rs Show resolved Hide resolved

Kijewski approved these changes Apr 21, 2022

View reviewed changes

djc merged commit b16085a into rinja-rs:main Apr 21, 2022

GuillaumeGomez deleted the automatic-trim branch April 21, 2022 08:36

GuillaumeGomez mentioned this pull request Apr 21, 2022

Trim all but 1 whitespace jinja condition #671

Closed

Kijewski mentioned this pull request Jan 14, 2023

More options for suppressing whitespaces #761

Open

djc mentioned this pull request Feb 26, 2023

0.12 release planning #722

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add suppress_whitespace config option and add `config` parameter to the derive macro #664

Add suppress_whitespace config option and add `config` parameter to the derive macro #664

GuillaumeGomez commented Apr 7, 2022

GuillaumeGomez commented Apr 8, 2022

djc left a comment

GuillaumeGomez commented Apr 11, 2022 •

edited

Loading

djc commented Apr 11, 2022

GuillaumeGomez commented Apr 11, 2022

GuillaumeGomez commented Apr 11, 2022

djc commented Apr 11, 2022

GuillaumeGomez commented Apr 13, 2022

GuillaumeGomez commented Apr 13, 2022

djc left a comment

djc commented Apr 13, 2022

GuillaumeGomez commented Apr 18, 2022

djc commented Apr 18, 2022

GuillaumeGomez commented Apr 18, 2022

vallentin left a comment

GuillaumeGomez commented Apr 19, 2022

vallentin commented Apr 19, 2022 •

edited

Loading

GuillaumeGomez commented Apr 19, 2022

vallentin commented Apr 19, 2022 •

edited

Loading

GuillaumeGomez commented Apr 19, 2022

Kijewski left a comment

GuillaumeGomez commented Apr 21, 2022

djc commented Apr 21, 2022

Add suppress_whitespace config option and add config parameter to the derive macro #664

Add suppress_whitespace config option and add config parameter to the derive macro #664

Conversation

GuillaumeGomez commented Apr 7, 2022

GuillaumeGomez commented Apr 8, 2022

djc left a comment

Choose a reason for hiding this comment

GuillaumeGomez commented Apr 11, 2022 • edited Loading

djc commented Apr 11, 2022

GuillaumeGomez commented Apr 11, 2022

GuillaumeGomez commented Apr 11, 2022

djc commented Apr 11, 2022

GuillaumeGomez commented Apr 13, 2022

GuillaumeGomez commented Apr 13, 2022

djc left a comment

Choose a reason for hiding this comment

djc commented Apr 13, 2022

GuillaumeGomez commented Apr 18, 2022

djc commented Apr 18, 2022

GuillaumeGomez commented Apr 18, 2022

vallentin left a comment

Choose a reason for hiding this comment

GuillaumeGomez commented Apr 19, 2022

vallentin commented Apr 19, 2022 • edited Loading

GuillaumeGomez commented Apr 19, 2022

vallentin commented Apr 19, 2022 • edited Loading

GuillaumeGomez commented Apr 19, 2022

Kijewski left a comment

Choose a reason for hiding this comment

GuillaumeGomez commented Apr 21, 2022

djc commented Apr 21, 2022

Add suppress_whitespace config option and add `config` parameter to the derive macro #664

Add suppress_whitespace config option and add `config` parameter to the derive macro #664

GuillaumeGomez commented Apr 11, 2022 •

edited

Loading

vallentin commented Apr 19, 2022 •

edited

Loading

vallentin commented Apr 19, 2022 •

edited

Loading