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

Render with Display using autoref specialization #359

Merged
merged 1 commit into from
Dec 15, 2022

Conversation

simonask
Copy link
Contributor

This may not be something you're interested in merging. No hard feelings if you close this outright. :-)

I found for my use case that I needed to write display(...) in a lot of places, especially in cases where I was using external types (especially the many datetime formatting types from icu).

This PR adds autoref-based specialization to the html! macro such that it will choose a type's Render implementation if it is available, and use Display as a fallback.

The blanket impl of Render for T where T: Display was removed in #271 for good reasons, and I will try to justify this PR in terms of those reasons:

  • impl Render for borrows ?  #266 (Impl Render for borrows): This PR does not add any new blanket impls of Render or Display.
  • Performance: Due to specialization, integers can still be rendered using itoa.
  • Semantics: Arguably, Display is intended to provide a human-readable string. While this isn't necessarily tied to the HTML representation of the thing, it is not unreasonable to want to use the output of Display in HTML. This solution treats the output of Display as unescaped content (so it will be escaped before appending, by going through maud::display(...)). If a situation arises where the Display and Render implementations should be completely different, specialization allows this on a per-type basis.

The main drawback of this PR is that compiler errors are slightly more confusing when a type implements neither Render nor Display. I have tried to name the magic inner types used in the macro with that in mind.

Let me know what you think. :-)

@lambda-fairy
Copy link
Owner

lambda-fairy commented Dec 15, 2022

Thanks and sorry for the delay!

I didn't expect the solution to be so clean – seeing your take validates the approach for me.

Let's merge this for now and see what people think in the next release.

@lambda-fairy lambda-fairy merged commit 6cdf12b into lambda-fairy:main Dec 15, 2022
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