Skip to content
This repository was archived by the owner on Mar 11, 2025. It is now read-only.

Implement Responder trait for actix-web #104

Closed
wants to merge 4 commits into from

Conversation

ryanmcgrath
Copy link
Contributor

@ryanmcgrath 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());
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Collaborator

@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.

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 }
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can we depend on a stable version here, please?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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"] }
Copy link
Collaborator

Choose a reason for hiding this comment

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

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

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"] }
Copy link
Collaborator

Choose a reason for hiding this comment

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

This also needs to be reverted, I think.

@ryanmcgrath
Copy link
Contributor Author

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~

@ryanmcgrath
Copy link
Contributor Author

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
Collaborator

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

Ah, amazing! Thanks again!

@ryanmcgrath
Copy link
Contributor Author

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

@djc
Copy link
Collaborator

djc commented Jul 23, 2018

I just released 0.7.1. 👍

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants