-
Notifications
You must be signed in to change notification settings - Fork 15
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
Non-tokio runtime compatibility #46
base: master
Are you sure you want to change the base?
Conversation
if status.is_success() { | ||
Ok(response.body_json().await?) | ||
} else { | ||
Err(Error::HttpError(StatusCode::from(u16::from(status)))) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does surf use http
crate status code? If yes I think we should drop the httpstatus dependency as well
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Surf uses http_types.
This would also create additional backwards incompatibility since it leaks out too in case that matters. Same group that created surf.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, I think we should take the opportunity of making a breaking change and think about other good changes. My understanding is that http
is the most standard source of basic http types. httpstatus
is definitely not it. Do you think http_types is a good idea to be the type we export out of this crate's error type?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
On the topic of making breaking changes, scryfall itself makes breaking changes all the time, so this crate has to make a lot of them as well. To be precise, wizards makes a lot of breaking changes 🤣
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can't say I have a ton of experience with it. I think it is better quality than httpstatus
but haven't done a thorough comparison either. From a quick survey, StatusCode
in http-types
and http
looks pretty similar.
Options:
http-types
- zero impedance mismatch withsurf
http
- most popular interfaceu16
- Remove the public facing dep entirely as just return au16
, let the client decide if they want to "interpret" it by pulling it into their favoriteStatusCode
object. Prevents consumers from being pinned to a specific version of the library too.
I actually think the last option might be the best.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should go with the last option them. Makes sense to me
Thanks for this PR, TIL about surf! For my own learning, I'm curious about your motivation for this PR, I myself only use Tokio, tried async-std once but never really used, can you tell me some reasons for preferring async-std over Tokio? |
@mendess The main motivation is to use this within a bevy app. It is not directly compatible with Tokio but uses it's own internal executor that is compatible with standard futures. Surf doesn't specifically rely on tokio or async-std but at least enables compatibility with either, in addition to any other executor that uses standard futures (such as in the case of bevy). I'm no expert in this but this is what I have concluded from my time boxed investigation. |
We could also make a larger refactoring to the API and add a |
I really like this idea. This was my first real rust project so it naturally misses some good ideas haha. Feel free to make a PR with this change! And thanks for the idea! |
I will see if I have the time and interest to work on it. No promises. |
Why?
Make scryfall-rs usable under non-tokio async runtimes.
Reqwest is only compatible with tokio. Surf is compatible with async-std, tokio, or any other standard futures compliant runtime.
seanmonstar/reqwest#719 (comment)
Known Issues
This will break backwards compatibility in the API. Specifically in regards to the removal of the
ReqwestError
variant.It is not abstracted in
HttpClientError
as astring
so going forward, any changes to the http client shouldn't have an impact on the public API.