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

impl Responder for actix-web framework #136

Merged
merged 2 commits into from
Jun 15, 2018

Conversation

anxiousmodernman
Copy link
Contributor

It looks similar to the Rocket implementation, and functionally is also similar:
a syncronous response for PreEscaped

Refs #135

@anxiousmodernman
Copy link
Contributor Author

If/when this looks good, I can write something up for the book https://github.com/lfairy/maud-book

It looks similar to the Rocket implementation, and functionally is also similar:
a syncronous response for PreEscaped<String>

Refs lambda-fairy#135
@anxiousmodernman
Copy link
Contributor Author

anxiousmodernman commented Jun 12, 2018

Ah the infamous "cannot link ring" more than once. My understanding is that we need separate test builds for separate features. Bummer. Hopefully that gets fixed this year. Update I added that in, but I don't know if it's acceptable.

Copy link
Owner

@lambda-fairy lambda-fairy left a comment

Choose a reason for hiding this comment

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

I was going to mention the book! Thanks for taking the initiative.


Overall the changes look good. The linking issue is unfortunate but should hopefully be temporary. I'm happy to merge this once the nits are addressed.

.travis.yml Outdated
- cargo test --all --all-features
- cargo test --all --features="iron"
- cargo test --all --features="rocket"
- cargo test --all --features="actix-web"
Copy link
Owner

@lambda-fairy lambda-fairy Jun 13, 2018

Choose a reason for hiding this comment

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

Can these lines use cargo build instead? With one cargo test --all at the end.

Since the web framework integrations aren't covered by the test suite, we can avoid clutter by only running the tests once.

maud/src/lib.rs Outdated
impl Responder for PreEscaped<String> {
type Item = HttpResponse;
type Error = Error;
fn respond_to<String>(self, _req: &HttpRequest<String>) -> Result<Self::Item, Self::Error> {
Copy link
Owner

Choose a reason for hiding this comment

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

This is better written as

fn respond_to<S>(self, _req: &HttpRequest<S>) -> ...

to avoid confusion with the built-in String type

Compiling rocket and actix-web together presents problems, since ring - a
native dependency - cannot have more than one version linked.
@max-frai
Copy link

@lfairy Can you merge this into master now? Because I use same implementation locally :)

@lambda-fairy lambda-fairy merged commit 8ce98f6 into lambda-fairy:master Jun 15, 2018
@lambda-fairy
Copy link
Owner

LGTM, thanks!

@anxiousmodernman anxiousmodernman deleted the actix_web_feature branch June 24, 2018 02:56
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.

3 participants