search

maxjoehnk / soundcloud-rs

SoundCloud API implementation in Rust
Apache License 2.0
4 stars 3 forks source link

fix track download and test, add example #1

Closed aaronenberg closed 4 years ago

aaronenberg commented 4 years ago

I'm very new to Rust and async programming so this was quite a fun challenge.

aaronenberg commented 4 years ago

I'm unaware of what to use in place of futures::io::AllowStdIo . That is needed to allow std::fs::File to be accepted in a context which requires futures::io::AsyncWrite . Maybe I'm just missing something so let me know and I've got all the other changes ready.

maxjoehnk commented 4 years ago

I'm unaware of what to use in place of futures::io::AllowStdIo . That is needed to allow std::fs::File to be accepted in a context which requires futures::io::AsyncWrite . Maybe I'm just missing something so let me know and I've got all the other changes ready.

Oh I was thinking of using tokio::fs::File as we've talked about the api compatibility with the tokio read trait. I thought it would be nice to demonstrate this use case in the doc tests or example.

aaronenberg commented 4 years ago

Ok no problem.

BTW I've been meaning to point out an issue with download/stream expecting a futures::io::AsyncWrite and read_url making the call to futures::io::copy. These together cause an inconvenience to the library user because they have to create a file before they even know the track can be fetched.

Using download as an example, one solution is to replace the writer argument with an optional path and create a file from this path if track.downloadable is true. If the path is None we can attempt to create a file in the current directory. This file get's passed into read_url's writer. Of course then we'd need to figure out: "does the caller want a tokio::fs::File or a std::fs::File?"

An even better solution would be just to remove the writer parameter and the call to futures::io::copy from read_url and return byte stream or an error. The user can create their own file after checking they actually got some data.

maxjoehnk commented 4 years ago

Well yeah, but the advantage of the current api is the user doesn't have to write to a file. Returning a byte stream would provide the same benefit, but I don't think there is a "standardized" interface for byte streams right now? I know of futures::Stream but I don't think it is a good fit for this api. Also the user still has the option to check track.downloadable or track.streamable (which is what I'm doing in the application I'm using this crate for. So I think its okay to leave the api as is.

aaronenberg commented 4 years ago

I agree byte streams are not user friendly at the moment. But I feel there should be distinction between the download and stream methods which currently have the same behavior. It's safe to assume a user expects a newly created file on their OS from the download method. Why make them go through the rigmarole of creating that file?

maxjoehnk commented 4 years ago

I'll give you that. But we should probably feature gate the download method then as tokio is quite a big dependency (even though we already depend on it through reqwest).

aaronenberg commented 4 years ago

Also I have some code for users and pagination that I wanted push but was waiting for this to get merged. Looks like you've started something that I've been working on.

maxjoehnk commented 4 years ago

Ah yes, I've noticed the branch while testing this PR yesterday but while implementing the user apis today I forgot which apis you already had implemented. Pagination would actually be quite nice. This is something I always skip while implementing api clients because it's just annoying to deal with.

maxjoehnk commented 4 years ago

I agree byte streams are not user friendly at the moment. But I feel there should be distinction between the download and stream methods which currently have the same behavior. It's safe to assume a user expects a newly created file on their OS from the download method. Why make them go through the rigmarole of creating that file?

I'll merge this now and we can discuss any api changes regarding file download in a separate issue. Is that okay for you?

aaronenberg commented 4 years ago

Sounds good to me.