Skip to content

Use ISO 646 alternative operators for bit ops + proper error messages for spaces around the |filter operator#18

Merged
GuillaumeGomez merged 4 commits into
askama-rs:masterfrom
Kijewski:pr-iso646
Jun 24, 2024
Merged

Use ISO 646 alternative operators for bit ops + proper error messages for spaces around the |filter operator#18
GuillaumeGomez merged 4 commits into
askama-rs:masterfrom
Kijewski:pr-iso646

Conversation

@Kijewski
Copy link
Copy Markdown
Member

@Kijewski Kijewski commented Jun 19, 2024

This change allows simplifying the use of filter expressions, because you won't have to care about spaces around the | pipe operator.

Todo:

Resolves #16.

Comment thread testing/tests/whitespace.rs Outdated
template.nested_1.nested_2.array = &["a0", "a1", "a2", "a3"];
template.nested_1.nested_2.hash.insert("key", "value");
assert_eq!(template.render().unwrap(), "\n0\n0\n0\n0\n\n\n\n0\n0\n0\n0\n0\n\na0\na1\nvalue\n\n\n\n\n\n[\n "a0",\n "a1",\n "a2",\n "a3"\n]\n[\n "a0",\n "a1",\n "a2",\n "a3"\n][\n "a0",\n "a1",\n "a2",\n "a3"\n]\n[\n "a1"\n][\n "a1"\n]\n[\n "a1",\n "a2"\n][\n "a1",\n "a2"\n]\n[\n "a1"\n][\n "a1"\n]1-1-1\n3333 3\n2222 2\n0000 0\n3333 3\n\ntruefalse\nfalsefalsefalse\n\n\n\n\n\n\n\n\n\n\n\n\n\n");
assert_eq!(template.render().unwrap(), "\n0\n0\n0\n0\n\n\n\n0\n0\n0\n0\n0\n\na0\na1\nvalue\n\n\n\n\n\n[\n "a0",\n "a1",\n "a2",\n "a3"\n]\n[\n "a0",\n "a1",\n "a2",\n "a3"\n][\n "a0",\n "a1",\n "a2",\n "a3"\n]\n[\n "a1"\n][\n "a1"\n]\n[\n "a1",\n "a2"\n][\n "a1",\n "a2"\n]\n[\n "a1"\n][\n "a1"\n]1-1-1\n3333 3\n2222 2\n0000\n3333\n\ntruefalse\nfalsefalsefalse\n\n\n\n\n\n\n\n\n\n\n\n\n\n");
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The diff here is worrying.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I changed the amount of tests in https://github.com/rinja-rs/rinja/pull/18/files#diff-32df128cd4727b02c717b5945bc12d3fb10a618ce4f7dc034e507fb91409cc7eL37-L38. I'll re-add another test to both lines in the template, so the asserted text does not change.

I don't quite like this kind of testing anyway, because it can't be debugged. Either the data is the same or not. It's a blackbox.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed but it's quite important sadly. It was very useful for me when debugging block filters. If you have a better idea to compare outputs, I'm very interested. :)

Comment thread rinja_parser/src/expr.rs Outdated
Comment thread rinja_parser/src/lib.rs Outdated
Comment thread rinja_parser/src/expr.rs Outdated
{{1*2}}{{ 1*2 }}{{ 1 *2 }}{{ 1* 2 }} {{ 1 * 2 }}
{{1&2}}{{ 1&2 }}{{ 1 &2 }}{{ 1& 2 }} {{ 1 & 2 }}
{{1|2}}{{ 1|2 }}{{ 1 |2 }}{{ 1| 2 }} {{ 1 | 2 }}
{{1 bitor 2}}{{ 1 bitor 2 }}{{ 1 bitor 2}}{{1 bitor 2 }} {{1 bitor 2}}
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why this weird tab change? Wouldn't adding whitespace characters before 1 and after 2 provide the expected result?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I had no idea what to do in the last test. :) If I remove it, testing/tests/whitespace.rs:42 changes.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh well, let's keep it as is for now, I'll try to take a look later on.

@Kijewski Kijewski force-pushed the pr-iso646 branch 3 times, most recently from 7efe77e to 6d704c3 Compare June 19, 2024 20:05
Comment thread rinja_parser/src/lib.rs Outdated
))(j)
} else {
Err(nom::Err::Failure(ErrorContext::new(
"The filter operator `|` must not be preceeded by any whitespace characters.\n\
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Little nit: other error messages don't start with a capital letter.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was not sure about that, because it's two sentences. Maybe I should join them with a ";", so an initial lower cases character makes more sense? :D

Comment thread rinja_parser/src/expr.rs Outdated
Comment thread rinja_parser/src/expr.rs Outdated
Comment thread rinja_parser/src/lib.rs Outdated
@@ -0,0 +1,46 @@
use rinja::Template;
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for adding the ui tests for error output!

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I love UI tests, and I love that it's so simple to do them in rust. In most languages you only test the "good" outcomes, but the bad outcomes are just as important.

@GuillaumeGomez
Copy link
Copy Markdown
Collaborator

So remains nitpicking and the docs. Do you me want to handle the cherry-picking?

@Kijewski
Copy link
Copy Markdown
Member Author

So remains nitpicking and the docs. Do you me want to handle the cherry-picking?

Yes, I'd be thankful if you do it.

@Kijewski Kijewski changed the title Use ISO 646 alternative operators for bit ops + allow spaces around the |filter operator Use ISO 646 alternative operators for bit ops + proper error messages for spaces around the |filter operator Jun 19, 2024
@GuillaumeGomez
Copy link
Copy Markdown
Collaborator

Say no more, I'm on it!

Kijewski added 3 commits June 23, 2024 20:47
This change allows simplifying the use of filter expressions, because
you won't have to care about spaces around the `|` pipe operator.
@Kijewski Kijewski marked this pull request as ready for review June 23, 2024 19:09
@GuillaumeGomez
Copy link
Copy Markdown
Collaborator

Looks good to me, thanks!

@GuillaumeGomez GuillaumeGomez merged commit fd1108c into askama-rs:master Jun 24, 2024
@Kijewski Kijewski deleted the pr-iso646 branch June 25, 2024 08:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Simplify syntax by removing all binary operations (like |/&/^)

2 participants