Skip to content
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

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

Merged
merged 4 commits into from
Jun 24, 2024

Conversation

Kijewski
Copy link
Collaborator

@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.

@@ -39,7 +39,7 @@ fn test_extra_whitespace() {
let mut template = AllowWhitespaces::default();
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
Contributor

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
Collaborator 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
Contributor

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. :)

@@ -35,7 +35,7 @@
{{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|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
Contributor

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
Collaborator 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
Contributor

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
))(j)
} else {
Err(nom::Err::Failure(ErrorContext::new(
"The filter operator `|` must not be preceeded by any whitespace characters.\n\
Copy link
Contributor

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
Collaborator 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

@@ -0,0 +1,46 @@
use rinja::Template;
Copy link
Contributor

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
Collaborator 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
Contributor

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

@Kijewski
Copy link
Collaborator 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
Contributor

Say no more, I'm on it!

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
Contributor

Looks good to me, thanks!

@GuillaumeGomez GuillaumeGomez merged commit fd1108c into rinja-rs:master Jun 24, 2024
17 checks passed
@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