-
Notifications
You must be signed in to change notification settings - Fork 124
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
[Auth restructure 1] Separate http into a new crate #213
Conversation
As for this point, |
It's a bit complicated/cumbersome to do so... For now I guess you can just run the tests locally for the Can we first talk about how cross-compilation isn't really necessary for the CI? I don't see what could possibly cause a fail in one architecture that doesn't happen in another; we don't have architecture-specific code. If we simplify that then we might be able to have a flow per crate as you suggest, but otherwise with 4 crates, 3 architectures and 2 possible configurations (ureq and reqwest) we would have a total of 4 * 3 * 2 = 24 CI flows just for the tests... Which would grow even bigger if we added more HTTP clients. |
Yeah, cross-compilation isn't high priority, so it's ok to move these cross-compilation CI flow, and doesn't need to compile for different architectures. |
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.
Beside the nitpick I mentioned, everything looks good to me.
Finally, the reviews of all PR are done :)
rspotify-http/src/common.rs
Outdated
Io(#[from] std::io::Error), | ||
} | ||
|
||
pub type Result<T> = std::result::Result<T, Error>; |
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.
Is it a better choice to have new names instead Error
and Result
, since it's conflicting with the type defined in standard library, might cause a bit of confusion.
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.
Most libraries also use Error
, you can then use it as http::Error
to avoid ambiguity wherever needed. The thing is that we'd have to change ClientError
to be rspotify::Error
as well for consistency. But seeing that we have ApiError
as well it might be more clear by using a prefix. I'll change it to HttpError
, but if you think the other option is better let me know and I'll undo it.
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.
HttpError
is good enough :)
Part 1 of splitting up #207.
This moves the http logic into its own crate. It should be able to compile by itself (but not
rspotify
itself)