Skip to content

Relax Sized constraint on impl FastWritable for Cow#432

Merged
Kijewski merged 5 commits intoaskama-rs:masterfrom
C0D3-M4513R:patch-1
May 7, 2025
Merged

Relax Sized constraint on impl FastWritable for Cow#432
Kijewski merged 5 commits intoaskama-rs:masterfrom
C0D3-M4513R:patch-1

Conversation

@C0D3-M4513R
Copy link
Copy Markdown
Contributor

Fixes: #431

Please note, that I have not actually tested this commit.
This should however address the issue.

@GuillaumeGomez
Copy link
Copy Markdown
Collaborator

Please add test to ensure it works as expected. Then seems all good to me.

@C0D3-M4513R
Copy link
Copy Markdown
Contributor Author

C0D3-M4513R commented May 4, 2025

I added a test, but for me the test-suite is currently in a failing state?
The ui test from askama_tests doesn't pass, as well as some other doc-tests.

Is that state expected?

Also for some reason escaping is broken? (see todo in the added "test")

Edit: The error messages have slightly more information for me it seems.
e.g.

the trait `PluralizeCount` is not implemented for `str`

in master vs

the trait `PluralizeCount` is not implemented for `str`, which is required by `&str: PluralizeCount`

for me.

Edit2:
It took way to long for me to realize, that escaping ONLY works with Display (not with FastWritable).

Signed-off-by: C0D3 M4513R <28912031+C0D3-M4513R@users.noreply.github.com>
// If it didn't, compiling the tests would fail.
//
// Since this test fails at compile-time, there isn't actually any need to run it.
#[allow(dead_code)]
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.

Suggested change
#[allow(dead_code)]
#[test]

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

The test works compile time though. I could even just put it in a const block.

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.

True, however it also allows us to ensure that it's actually built.

Copy link
Copy Markdown
Contributor Author

@C0D3-M4513R C0D3-M4513R May 4, 2025

Choose a reason for hiding this comment

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

Why would the test not be built?
With cargo test --workspace it's built.

Comment thread testing/tests/cow_str_implements_fast_writable.rs Outdated
Co-authored-by: René Kijewski <Kijewski@users.noreply.github.com>
Copy link
Copy Markdown
Member

@Kijewski Kijewski left a comment

Choose a reason for hiding this comment

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

Thank you!

@Kijewski Kijewski merged commit c31fce7 into askama-rs:master May 7, 2025
39 checks passed
@C0D3-M4513R
Copy link
Copy Markdown
Contributor Author

Thank you for the better test code

@C0D3-M4513R C0D3-M4513R deleted the patch-1 branch May 7, 2025 15:12
@Kijewski Kijewski mentioned this pull request Jun 6, 2025
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.

Cow<'_, str> doesn't implement FastWritable

3 participants