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

Remove rinja_escape and make |escape a "normal" filter #53

Merged
merged 4 commits into from
Jul 7, 2024

Conversation

Kijewski
Copy link
Collaborator

@Kijewski Kijewski commented Jul 7, 2024

rinja_escaped was a somewhat odd beast: Some functions and types were unused, some implementation details were open for the world to see, and it was checked at runtime if a value was safe when it could be deduced at compile type. Now |escape and |safe are proper filters with nothing unusual about them, and the additional crate is gone.

This PR is a first step towards #51.

@Kijewski
Copy link
Collaborator Author

Kijewski commented Jul 7, 2024

$ cd testing
$ cargo bench --bench all
Big table               time:   [345.95 µs 348.52 µs 351.07 µs]
                        change: [-6.9603% -4.5108% -2.0227%] (p = 0.00 < 0.05)
                        Performance has improved.

Teams                   time:   [479.11 ns 480.44 ns 481.86 ns]
                        change: [-9.7181% -8.6280% -7.9141%] (p = 0.00 < 0.05)
                        Performance has improved.

@GuillaumeGomez
Copy link
Contributor

Looks all good to me, just one thing: could you add a test an invalid escaper please? I didn't see one. Would be also nice to improve the error message from invalid escaper for escape filter to invalid escaper `{provided_escaper_name}` for `escape` filter.

@GuillaumeGomez
Copy link
Contributor

Same for no escaper defined for extension '{escaping}'.

@Kijewski
Copy link
Collaborator Author

Kijewski commented Jul 7, 2024

Looks all good to me, just one thing: could you add a test an invalid escaper please?

Sure! I updated the error message to contain the unknown escaper, and added a UI test.

@GuillaumeGomez
Copy link
Contributor

CI needs to be updated too.

|
= note: this error originates in the derive macro `Template` (in Nightly builds, run with -Z macro-backtrace for more info)

error: no escaper defined for extension 'tex'
Copy link
Contributor

Choose a reason for hiding this comment

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

Thinking more about this error message, I don't think it makes much sense, especially for users who just did a typo. What do you think about unknown extension 'tex'. The available extensions are 'x', 'y', 'z'. What do you think?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Good idea! Added.

@Kijewski
Copy link
Collaborator Author

Kijewski commented Jul 7, 2024

CI needs to be updated too.

It is, but I failed to cargo fmt the source. :)

f.write_str("The available extensions are:")?;
for (i, mut e) in exts.iter().copied().enumerate() {
if e.is_empty() {
e = r#""" <empty string>"#;
Copy link
Contributor

Choose a reason for hiding this comment

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

I thought it was a debug display before checking. XD

I think using parens would be better here:

Suggested change
e = r#""" <empty string>"#;
e = r#""" (empty string)"#;

One extra thought: wouldn't it be better to rely on join instead? So your first iterator could become:

let exts: String = self
            .0
            .iter()
            .flat_map(|(exts, _)| exts)
            .map(|x| if x.is_empty() { r#""" (empty string)"# else { x.as_ref() })
            .collect::<BTreeSet<&str>>()
            .into_iter()
            .join(", ");

No need for the loop below like this. And I think perf is not really a big issue here since we're errorring.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

You're right. :) I use slice::join and the debug display of the extension, now.

.iter()
.flat_map(|(exts, _)| exts)
.map(|x| format!("{x:?}"))
.collect::<Vec<_>>();
Copy link
Contributor

Choose a reason for hiding this comment

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

Even better. 👍

GuillaumeGomez
GuillaumeGomez previously approved these changes Jul 7, 2024
@GuillaumeGomez
Copy link
Contributor

Once merge conflict is fixed, please merge.

@Kijewski Kijewski merged commit d401de0 into rinja-rs:master Jul 7, 2024
17 checks passed
@Kijewski Kijewski deleted the pr-simplify-escape branch July 7, 2024 21:23
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.

2 participants