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

Implement Responder trait for actix-web #104

Closed
wants to merge 4 commits into from
Closed

Conversation

@ryanmcgrath
Copy link
Contributor

ryanmcgrath commented Jul 21, 2018

Heya!

Wound up needing this and figured I'd throw it over as a pull request. It's built for actix-web 0.7 which went out less than 12 hours ago, but I originally built it against 0.6.x and it should work there too ('s why I include the mime_guess crate rather than rely on actix-web's fs::{} methods).

Test passes, but I couldn't figure out how you were opting to run against different web frameworks (Iron/Rocket/etc) so it needs to be enabled to run. Hoping this is the ~90% that lets you do the last 10% to make it correct per your package design, or something.

Cheers!


// Implement Actix-web's `Responder`.
fn impl_actix_web_responder(&mut self) {
//let lifetime = syn::Lifetime::new("'askama", Span::call_site());

This comment has been minimized.

@ryanmcgrath

ryanmcgrath Jul 21, 2018 Author Contributor

Bah, I forgot to remove these. They can be taken out,actix_web::Responder doesn't need 'em.

Copy link
Owner

djc left a comment

Great stuff, I've been wanting to get this in!

I have a number of small things that I'd like to improve before I merge this. I can do it some time soon if you don't want to spend more time on it, but if you can that would be much appreciated.


[dependencies]
askama_derive = { path = "../askama_derive", version = "0.7.0" }
askama_shared = { version = "0.7.0", path = "../askama_shared" }
iron = { version = ">= 0.5, < 0.7", optional = true }
rocket = { version = "0.3", optional = true }
actix-web = { version = ">= 0.6", optional = true }
mime_guess = { version = "2.0.0-alpha", optional = true }

This comment has been minimized.

@djc

djc Jul 21, 2018 Owner

Can we depend on a stable version here, please?

This comment has been minimized.

@ryanmcgrath

ryanmcgrath Jul 21, 2018 Author Contributor

Ah, this 2.0.0-alpha? That's...

I know, I found it weird too.

@@ -9,15 +9,17 @@ default = []
nightly = ["rocket", "rocket_codegen", "askama/with-rocket"]

[dependencies]
askama = { path = "../askama", version = "*", features = ["with-iron", "serde-json"] }
askama = { path = "../askama", version = "*", features = ["iron", "serde-json"] }

This comment has been minimized.

@djc

djc Jul 21, 2018 Owner

I think this change is probably responsible for the test failures, the feature in askama is still called with-iron here, right?

This comment has been minimized.

@djc

djc Jul 21, 2018 Owner

Also, I'd like to have the Cargo manifest changes for the testing crate in the earlier test commit. I can take care of that if you prefer, though.

serde_json = "1.0"

[build-dependencies]
askama = { path = "../askama", version = "*", features = ["with-iron", "serde-json"] }
askama = { path = "../askama", version = "*", features = ["iron", "serde-json"] }

This comment has been minimized.

@djc

djc Jul 21, 2018 Owner

This also needs to be reverted, I think.

@ryanmcgrath
Copy link
Contributor Author

ryanmcgrath commented Jul 21, 2018

Hmm, it'd probably be a bit before I could circle back to these (and to be honest, all the testing stuff confused me slightly here, so I'd honestly defer to you on it). Let me know if you need anything more from me though? Glad it's (mostly) there~

…r the template. Surprisingly useful.
@ryanmcgrath
Copy link
Contributor Author

ryanmcgrath commented Jul 22, 2018

One other thing I added over in my fork that I found useful - a method on the Template trait that returns the file extension for the template. It sounds weird, but it comes in handy for getting around issues in frameworks where you want to return a variety of different things from a request handler, and need to turn it into an HTTP Response object - thus needing the extension to determine the mimetype.

@djc
Copy link
Owner

djc commented Jul 22, 2018

I've merged all this stuff with a bunch of tweaks: c860bee for the actix-web Responder impl, ed98793 for the tests, and 8a25a1e to add an extension() method to Template.

Thanks again!

@djc djc closed this Jul 22, 2018
@ryanmcgrath
Copy link
Contributor Author

ryanmcgrath commented Jul 22, 2018

Ah, amazing! Thanks again!

@ryanmcgrath
Copy link
Contributor Author

ryanmcgrath commented Jul 22, 2018

Oh, quick question - when do you plan on throwing this up to crates.io?

@djc
Copy link
Owner

djc commented Jul 23, 2018

I just released 0.7.1. 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants
You can’t perform that action at this time.