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 routerify integration #487

Closed
wants to merge 1 commit into from
Closed

Add routerify integration #487

wants to merge 1 commit into from

Conversation

dvtkrlbs
Copy link

closes #484

@dvtkrlbs
Copy link
Author

This can also be generalized as a hyper integration since there is not any routerify specific code here.

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.

Since the implementation does not require routerify at all, let's call the crate askama_hyper and rename all the bits in other Askama crates to hyper. Then rename the test file to routerify.rs (and please just merge support.rs into it.)

readme = "README.md"
edition = "2018"

# See more keys and their definitions at https://doc.rust-lang.org/cargo/reference/manifest.html
Copy link
Owner

Choose a reason for hiding this comment

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

Please get rid of this.

buf.writeln(&format!("::askama_routerify::respond(&self, {:?})", ext))?;
buf.writeln("}")?;
buf.writeln("}")
}
// Writes header for the `impl` for `TraitFromPathName` or `Template`
Copy link
Owner

Choose a reason for hiding this comment

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

Add an empty line here below the new method?

[dependencies]
askama = { version = "0.10.6", path = "../askama", default-features = false, features = ["with-routerify", "mime", "mime_guess"] }
hyper = "0.14"
routerify = "2.1"
Copy link
Owner

Choose a reason for hiding this comment

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

Also, move routerify to be a dev dep.

Copy link
Author

Choose a reason for hiding this comment

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

If we turn this to askama_hyper I think we can drop it completely

Copy link
Owner

Choose a reason for hiding this comment

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

Works for me!

@dvtkrlbs
Copy link
Author

Did most of the stuff but the problem is we need the client feature of hyper on testing. It looks like testing only features is a open issue on cargo repo. So I don't know how to test the hyper code. Any idea?

@djc
Copy link
Owner

djc commented Jun 15, 2021

Sorry, not sure what you mean, can you give me more context for what you're trying to do?

@dvtkrlbs
Copy link
Author

We test the http framework integrations and testing actix one is kinda hard since we need some kind of client or optionally activate the client feature on tests (i dont think this one is possible).

@djc
Copy link
Owner

djc commented Jun 28, 2021

You can just repeat the dependency in the dev-dependencies with the extra feature added?

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.

[feature] Routerify
2 participants