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

Breaking Change: replace Reject trait with Into<Rejection> #527

Closed
wants to merge 7 commits into from

Conversation

yaahc
Copy link

@yaahc yaahc commented Apr 8, 2020

I'm not sure this is the best approach yet but the goal rn it to make it so warp can support 3rd party error types like anyhow and eyre without those types having to have a conditional dependency on warp just to impl the Reject trait.

@yaahc
Copy link
Author

yaahc commented Apr 8, 2020

do not merge

This trait doesn't yet solve the problem I care about, in fact it makes it worse, I still haven't been able to figure out how to have Rejection support wrapping any type that other than itself that is Debug + Sized + Send + Sync + 'static

The best idea I have rn is to just drop the Debug impl on Rejection and expose its debug impl through a member function, which would allow it to impl From<Debug + Sized + Send + Sync + 'static> without overlapping with From<T> for T, and then the from impl could be used whenever we create a custom, which would prevent it from double wrapping itself

@seanmonstar
Copy link
Owner

One thing to note is that the rejection system in warp is purposefully not meant to be std::error::Error. The extra trait is to make one think of meaningful rejections, that you then look for in recover.

@yaahc yaahc changed the title Breaking Change: Switch from Reject to Error trait Breaking Change: replace Reject trait with Into<Rejection> Apr 8, 2020
@yaahc
Copy link
Author

yaahc commented Apr 8, 2020

well that works out because basing it on std error was not possible without specialization afaik. The "remove Debug on Rejection" solution ended up working, bonus points is now when type inferences lets you you can use ? to go straight into a Rejection

@yaahc
Copy link
Author

yaahc commented Apr 8, 2020

Got it working and passing tests, not being able to just unwrap() a Rejection because it no longer impls Debug sure is annoying in test code :/

@seanmonstar
Copy link
Owner

Thanks for the PR! I'm inclined to keep the Reject trait separate for now, as that design was on purpose (more can be found in the related PRs and commits). If that purpose turns out to be flawed in practice, we could just resort to std::error::Error, but I'd suggest we have a discussion with the reasons why that's better in an issue first.

@yaahc
Copy link
Author

yaahc commented May 18, 2020

That's fair, I probably wont push on this because I was only using warp for a small side project that ended up getting discarded but if someone else ends up asking about it or said Reject trait bikeshed starts happening please let me know and ill pop in and explain/defend the changes in this PR and why I think they're helpful.

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