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 blanket Render impl for T: Display #271

Closed
lambda-fairy opened this issue Apr 24, 2021 · 0 comments · Fixed by #320
Closed

Remove blanket Render impl for T: Display #271

lambda-fairy opened this issue Apr 24, 2021 · 0 comments · Fixed by #320
Assignees
Milestone

Comments

@lambda-fairy
Copy link
Owner

lambda-fairy commented Apr 24, 2021

While this is convenient, it's problematic because:

  1. Weird edge cases. See impl Render for borrows ?  #266. While that particular case could be solved with an impl Render for &PreEscaped<T>, I'd rather go for a more general solution.
  2. Performance. The std::fmt machinery is pretty heavyweight, and building Render on top of it locks us out of making it faster. In fact, this is the reason why markup.rs beats Maud in benchmarks – they use the itoa crate to render integers.
  3. Semantics. There's no particular reason why the text and HTML representations should be tied together.

As part of this change, we should:

  • Add manual impls for integer types.
  • Investigate impact on users like miniserve (and maybe send them a migration PR)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant