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.

Sign up for GitHub

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

Jump to bottom

Change the T: Display constraint on PreEscaped to something else #54

Closed
lambda-fairy opened this issue Oct 18, 2016 · 3 comments
Closed

Change the T: Display constraint on PreEscaped to something else #54

lambda-fairy opened this issue Oct 18, 2016 · 3 comments
Labels
question

Comments

@lambda-fairy
Copy link
Owner

lambda-fairy commented Oct 18, 2016 •
edited
Loading

Using Display here seems wrong, because semantically Display gives a plain text representation, not an HTML representation. So in effect, PreEscaped<T: Display> is a claim that the plain text and HTML representations of a type are the same, which doesn't really make sense in general.

We can change the constraint to T: AsRef<str> instead. This covers what I think is the primary use case (including HTML output from an external source) without bringing in that semantic baggage. It doesn't cover fancy data structures like ropes, but I don't think they'll come up in a web app.

This change will also drive users toward implementing the Render trait, which is the idiomatic way to customize HTML output (see #53).

My question is: Are there any existing use cases of PreEscaped which are not either...

  1. Including HTML output from an external source (e.g. a blog post converted from Markdown)
  2. PreEscaped("<!DOCTYPE html>")
  3. Anything which is better written using the Render trait instead

... ?

If not, then we can go ahead on this change.
@lambda-fairy lambda-fairy self-assigned this Oct 18, 2016
@lambda-fairy lambda-fairy removed their assignment Oct 18, 2016
@TheNeikos
Copy link
Contributor

Why not constrain on Render and remove the Blanket impl for Display ?

@TheNeikos
Copy link
Contributor

This would entail implementing it for all the primitives, but I guess it's feasible (with a macro it should be simple too)

@lambda-fairy
Copy link
Owner Author

lambda-fairy commented Oct 22, 2016 •
edited
Loading

@TheNeikos That is an interesting idea. I've mulled over the issue a bit, and I think the blanket impl<T: Display> Render for T is still useful (if that's what you mean).

For many types (e.g. IpAddr, Version, Url...) we do want to re-use the Display impl. It'd be annoying to write Render instances for all of these.

I also don't think the same argument works for PreEscaped<T>. If you know that an IP address never contains a special character, then the best way to show that is by specializing the Render impl. Not by asking everyone who uses IP addresses to wrap them in PreEscaped 😜

Given these points, I'm going ahead with the original plan.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
question
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
2 participants
@lambda-fairy @TheNeikos