Replace failure with anyhow and thiserror

[arlyon]

As described in #122 , failure is deprecated. This replaces it with anyhow and thiserror.

Closes #122

[marioortizmanero]

Thanks a lot for the PR!

I've been thinking if failure is necessary since I first saw it in the codebase. Do you think anyhow is worth it still? I read that these kind of libraries are often used for binaries, not libraries.

This is a good opportunity to just get rid of failure/anyhow. I'll investigate a bit about that when I have time.

[arlyon]

Hey, yeah I have the same thoughts. Ideally we'd only use thiserror (or nothing but it helps with boilerplate) so library consumers can error handle better but I figured this would be a good first step :)

[marioortizmanero]

I think we could remove anyhow in this same PR, if you're up for it. I like the Result type alias because it makes the endpoints more concise, so we could use an internal Result type as well (type Result = std::result::Result<T, ApiError>)

I don't personally use thiserror myself because I consider it unnecessary, but it is indeed quite nice. I would like an opinion from @ramsayleung, who is the main maintainer.

[ramsayleung]

I think we could remove anyhow in this same PR, if you're up for it.

I kind of agree with @marioortizmanero, anyhow is seems not that necessary, we could use an internal Result type as what Mario suggests: type Result = std::result::Result<T, ApiError>.

Oh the other hand, thiserror's doc also points out:

Use Anyhow if you don't care what error type your functions return, you just want it to be easy. This is common in application-like code.

As for thiserror, it is nice and acceptable, we could use it to reduce some hand-write boilerplate code.

[arlyon]

Sounds good to me. If you're both behind it I'll see what I can do!

[arlyon]

I've added some changes. Please have a look at the new error type and let me know what you think. Tried to follow thiserror convention when possible.

[arlyon]

I also have a branch with a few 'misc' changes like some clippy lints and code fixes that I came across while working on this, should I open another PR for those?

master...arlyon:misc-fixes

[ramsayleung]

I also have a branch with a few 'misc' changes like some clippy lints and code fixes that I came across while working on this, should I open another PR for those?

Yep, it would be better to open a new PR for these 'misc' changes, because there is nothing related to this PR: anyhow and thiserror. It could be much clear to open a new PR to illustrate what you wan to do :)

[arlyon]

This was rewritten because indexing directly can panic (though it is unlikely). Doing a match on

if let Some(duration) = response.headers().get(...).and_then(|header| header.to_str().ok()) {

was suddenly a lot going on on a single line so I used another and_then instead.

[marioortizmanero]

I agree, It's better this way IMO

[ramsayleung]

Yeah, it's more robust than before :)

[arlyon]

This was rewritten simply to match the same functional style as above for consistency.

[marioortizmanero]

Looks good to me! The only nitpick: IMO the "Error" suffix in the error types are unnecessary. ClientError already points out that its children are different types of errors. Unauthorized for instance would be enough on its own, because otherwise you have to use ClientError::UnauthorizedError. Thoughts on this?

Can you also add this in the CHANGELOG.md?

[arlyon]

After looking at it, you're right. The rust api guidelines recommend calling it BlaBlaError but that's refering to the type not the variants. I'll fix it and update the changelog.

[marioortizmanero]

Thanks for your help! This can probably be merged now (not sure if I should be merging PRs myself).

@ramsayleung, should we think about releasing a new version, or do we wait for #120? That one is going to take longer than I expected and there are quite a few breaking changes already. Or maybe it's better to release all the breaking changes at once? Not sure. We could create a new issue tracking the new release like many other libraries do, or at least use GitHub milestones.

[ramsayleung]

should we think about releasing a new version, or do we wait for #120?

I suggest to wait for #120, so we don't need to release a version of breake changes twice.

We could create a new issue tracking the new release like many other libraries do, or at least use GitHub milestones.

Yep, It's a good idea. I personnally suggest to create a milestone of stable version, and label this version as the one last version of breake change before the stable version.

After cleaning up the blocking module, and refacting codebase of error handle, I think we are marching to the stable version, rspotify will be stable enough to reach version 1.0

[ramsayleung]

Merged :)

[marioortizmanero]

Yep, It's a good idea. I personnally suggest to create a milestone of stable version, and label this version as the one last version of breake change before the stable version.

After cleaning up the blocking module, and refacting codebase of error handle, I think we are marching to the stable version, rspotify will be stable enough to reach version 1.0

I personally think we're nowhere near a stable version yet. After #120, there's still #116 and #109 with breaking changes. #4 might also include some breaking changes, and the newly opened #124 could change some things as well.

[ramsayleung]

Ooops, it still have some features needing to be implemented :)