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 support for warp #290

Merged
merged 1 commit into from Jan 29, 2020
Merged

Add support for warp #290

merged 1 commit into from Jan 29, 2020

Conversation

aeons
Copy link
Contributor

@aeons aeons commented Jan 23, 2020

Add support for Warp's Reply trait in template derivations.

@djc
Copy link
Owner

djc commented Jan 27, 2020

Hey, sorry for the slowish response to this!

I've been running into trouble from having all these web framework integrations included in my template engine crate (even if optionally). This causes issues because the web framework's dependencies leak into my dep graph, causing problems if conflicting dependencies are used by the web frameworks. So, while I'm open to hosting warp integrations in this repository, I think we'd need a different approach to integrations altogether. See more discussion in #234; my attempts so far have run aground on coherence issues.

Having the integration included in the web framework might be a more attractive solution, but I'm not sure if Sean et al would be open to that (do they already ship some templating integrations?). Alternatively, I need a sustainable way to do the integrations that will let me keep integrations up to date without running into dependency conflicts going forward.

@aeons
Copy link
Contributor Author

aeons commented Jan 27, 2020

That makes sense.

This feels like a place where orphan instances would be nice to have.

There are currently no templating integrations in the warp repo, but I can check if they would be interested.

I can't really tell whether the separate crate approach worked out or not? Is that something I should pursue for this?

@djc
Copy link
Owner

djc commented Jan 27, 2020

If you want to help figuring this out, that would be awesome. Currently I'm thinking that it might be better to have per-integration crates that have their own Template: askama::Template traits. Those integration crates could then also contain a blanket implementation that implements Reply (or equivalent) for traits that implement these. Then there should be some feature flag in the derive crate (and exposed through the API crate) that makes the code generator implement that extra trait.

@aeons
Copy link
Contributor Author

aeons commented Jan 27, 2020

I haven't updated the documentation, and the mime stuff is just copy/pasted, but this seems to work.

@djc
Copy link
Owner

djc commented Jan 27, 2020

This is great, thanks! I'd be happy to merge this once you fix up the documentation. Also any generic mime stuff can still live in askama if you like, that shouldn't be too big of an issue.

@djc
Copy link
Owner

djc commented Jan 27, 2020

Also, if possible, it would be nice to have the tests in the askama_warp crate.

@aeons
Copy link
Contributor Author

aeons commented Jan 27, 2020

Alright, I'll make the get_mime_type a pub fn then.
Looking at tests now.

@aeons
Copy link
Contributor Author

aeons commented Jan 27, 2020

Right now cargo test works in both askama_warp and testing, but when I run cargo test in the workspace, it enables the warp feature in askama_derive when running the tests in testing, which makes the compilation fail.

I can't figure out how to not enable that feature when running the workspace tests.

@aeons
Copy link
Contributor Author

aeons commented Jan 27, 2020

Helpfully, I found this issue rust-lang/cargo#6195
😆

@djc
Copy link
Owner

djc commented Jan 27, 2020

Recognizing that this should never come up in the wild, maybe there's a hack that could work where we add a non-additive feature to askama_derive that negates the warp integration. The testing crate would then depend on that feature. How do you feel about that?

@aeons
Copy link
Contributor Author

aeons commented Jan 27, 2020

Something like this?

if cfg!(feature = "warp") && !cfg!(feature = "test") {
  self.impl_warp_reply(&mut buf);
}

@djc
Copy link
Owner

djc commented Jan 27, 2020

Yes, something like this. I'd like to name the feature no-integrations though, in anticipation of using it for the other integrations in the same way.

@aeons
Copy link
Contributor Author

aeons commented Jan 27, 2020

That fixes testing, but now askama_warp fails since warp::reply::Reply is not implemented. I think it relates to the same problem you had in the linked issue.

@djc
Copy link
Owner

djc commented Jan 28, 2020

Have been thinking about this some more. What if we change the workspace's default-members to exclude askama_warp (and, in the future, other integration crates)? Can we then run cargo t --workspace and cargo t -p askama_warp successfully in separate invocations?

@aeons
Copy link
Contributor Author

aeons commented Jan 28, 2020

That seems to work, if you just do cargo test in the repo root.
cargo test --workspace fails with the same errors though.

@aeons
Copy link
Contributor Author

aeons commented Jan 28, 2020

I removed the --workspace from both build and test commands.

From cargo help build:

All packages in the workspace are built if the --workspace flag is supplied. The
--workspace flag is automatically assumed for a virtual manifest.
Note that --exclude has to be specified in conjunction with the --workspace flag.

It looks like --workspace makes it ignore the default-members.

@djc
Copy link
Owner

djc commented Jan 29, 2020

So I've redone all the other integrations as separate crates now. This has caused some churn, but hopefully rebasing your work should be straightforward -- and it provides a clear model on how to add more integrations. Thanks for working on this with me!

@djc
Copy link
Owner

djc commented Jan 29, 2020

(Also, please squash all your commits on this branch.)

@aeons
Copy link
Contributor Author

aeons commented Jan 29, 2020

This ended out nicely I think. Thanks.

@aeons
Copy link
Contributor Author

aeons commented Jan 29, 2020

The readme still needs to be updated I guess.

@djc djc merged commit 91c2bbf into djc:master Jan 29, 2020
@djc
Copy link
Owner

djc commented Jan 29, 2020

Thanks! I'll make sure to update the documentation before the next release. Are you comfortable with using the wrap integration from git, or would you prefer to have a release soonish?

@aeons aeons deleted the warp branch January 29, 2020 13:07
@aeons
Copy link
Contributor Author

aeons commented Jan 29, 2020

No rush, I'm just playing around with it for now.

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

2 participants