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

Unseal Wrap trait #640

Closed
wants to merge 6 commits into from
Closed

Conversation

hgzimmerman
Copy link
Contributor

Fixes #638.

The doc comments for the Wrap trait are not suitable for merging at the moment. Any feedback related to them would be appreciated.

Copy link
Collaborator

@jxs jxs left a comment

Choose a reason for hiding this comment

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

Hi, thanks for getting this started!

@@ -1,27 +1,21 @@
use super::Filter;

pub trait WrapSealed<F: Filter> {
/// Wraps filters.
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 give a detailed explanation of the trait, in the line of others like reply with a working example?

Copy link
Contributor Author

@hgzimmerman hgzimmerman Jul 8, 2020

Choose a reason for hiding this comment

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

I'm struggling to come up with an example use case. Most existing uses of the Wrap trait use some sealed structure, so I'm not sure what would make sense to demonstrate here.

If the wrapper needs to implement FilterBase then unsealing Wrap doesn't open up a huge amount of flexibility.

Copy link
Collaborator

Choose a reason for hiding this comment

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

it's getting nice! can you show where you are stalled? I think you can use BoxedFilter

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm struggling to come up with a terse, but plausible use for Wrap. I look to something like warp::filters::log::Log or warp::filters::reply::WithHeader and see that their Wrap implementation is relying on private items. I don't think BoxedFilters will make this task easier for items that are like Log or WithHeader

The ultimate motivation behind me wanting to unseal this trait is to be able to implement something like what is detailed in this comment: #408 (comment), but I see that that also relies on other private items.

So its a problem of 1. I don't know what the example should demonstrate. and 2. I don't know if a plausible example could be created with the other "primitives" that are exposed currently.

@hgzimmerman
Copy link
Contributor Author

Ok. So I finally figured out what you meant by using BoxedFilter.
I don't quite have it working yet due to some trait constraints, the errors of which should be present in the CI system, but I'll post the most notable ones here:

---- src/filter/wrap.rs - filter::wrap::Wrap (line 18) stdout ----
error[E0277]: the trait bound `<<F as warp::filter::FilterBase>::Extract as warp::generic::Tuple>::HList: warp::generic::Combine<_>` is not satisfied
##[error]  --> src/filter/wrap.rs:33:14
   |
18 |             .and(warp::any().map(|| log::info!("")))
   |              ^^^ the trait `warp::generic::Combine<_>` is not implemented for `<<F as warp::filter::FilterBase>::Extract as warp::generic::Tuple>::HList`

error[E0277]: the trait bound `std::convert::Infallible: warp::reject::sealed::CombineRejection<<F as warp::filter::FilterBase>::Error>` is not satisfied
##[error]  --> src/filter/wrap.rs:33:14
   |
18 |             .and(warp::any().map(|| log::info!("")))
   |              ^^^ the trait `warp::reject::sealed::CombineRejection<<F as warp::filter::FilterBase>::Error>` is not implemented for `std::convert::Infallible`

Any advice on where to go from here would be appreciated.

@jxs jxs mentioned this pull request Jul 15, 2020
@jxs
Copy link
Collaborator

jxs commented Jul 15, 2020

sorry for the late reply, and thanks for your effort! yeah this solution seems it would always need to unseal other traits which would lead to more confusion, and in the end even if it worked one would always need to impl Wrap and probably Future which would not be the way one works with Filters.
I have since posted an update on #16 (comment) can you give your opinion there?
Thanks

@hgzimmerman
Copy link
Contributor Author

Should this PR be closed in favor of your wrap_fn proposal?

@hgzimmerman
Copy link
Contributor Author

Is there anything I can do to assist in getting your wrap_fn included into Warp?

@jxs
Copy link
Collaborator

jxs commented Jul 22, 2020

@hgzimmerman yeah we can close this one in favor of the wrap_fn proposal, which i am awaiting for feedback on the issue.
Thanks

@jxs jxs closed this Jul 22, 2020
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.

Unseal Wrap trait
2 participants