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

Add TemplateResponder trait for integration with actix-web #205

Merged
merged 2 commits into from
Feb 20, 2019

Conversation

zizhengtai
Copy link
Contributor

This is similar to what actix_web::AsyncResponder does. It enables us to call .responder() on a template to render it into an actix_web::HttpResponse. This is advantageous over the current approach (implementing actix_web::Responder for T: Template) because the current approach doesn't allow returning templates with references to local variables. Please see the added test for what I mean.

@djc
Copy link
Owner

djc commented Jan 27, 2019

Should we remove the current approach, then, or does it address a different use case?

@zizhengtai
Copy link
Contributor Author

zizhengtai commented Jan 27, 2019 via email

@gregelenbaas
Copy link

I don't think the current approach should be removed. I like the addition for flexibility but I think it would cause unnecessary breakage as well as no longer allowing the shorter syntax when local variables do not need to be computed.

@djc
Copy link
Owner

djc commented Feb 12, 2019

Closing, no longer necessary (see #204).

@djc djc closed this Feb 12, 2019
@zizhengtai
Copy link
Contributor Author

@djc, I think there is a misunderstanding. This PR isn't doing the same thing as #204. #204 only provides a way to return a template without specifying the concrete return type (but instead with a impl Responder), but it doesn't solve the problem that the returned template cannot contain references to local string slices. The example in #204 compiles only because the reference has static lifetime; if you change it to a 'a (which is what this PR allows for), the code doesn't compile.

Could you please reopen this PR and review it? Thank you.

@djc djc reopened this Feb 12, 2019
@zizhengtai
Copy link
Contributor Author

Any updates on this?

@djc
Copy link
Owner

djc commented Feb 17, 2019

So as I understand we can only generate code for the new approach, remove the old respond() implementation and the code generation for the stuff that's there now, and almost all of the current users' code would stay working. Can someone explain to me why that might not be true?

Copy link
Owner

@djc djc left a comment

Choose a reason for hiding this comment

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

I'll just merge this and do some follow-ups.

@djc djc merged commit 4c4d09e into djc:master Feb 20, 2019
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.

None yet

3 participants