Skip to content

fix: map authentication errors to a custom error type #566

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Feb 6, 2021
Merged

fix: map authentication errors to a custom error type #566

merged 1 commit into from
Feb 6, 2021

Conversation

xou816
Copy link
Contributor

@xou816 xou816 commented Jan 18, 2021 •
edited
Loading

Fixes #565

Errors encountered during authenticate() are mapped to a custom error type with an ErrorKind enum (BadCredentials, PremiumAccountRequired, Io (wraps io errors) and Msg (generated, used as a default kind to keep unmapped errors readable)) instead of panicking.

I am not sure how backward compatibility is usually handled here however! I was thinking about adding this error mapping to a new Session::try_connect (not sure about the naming) function, while keeping the same behavior for Session::connect

@xou816 xou816 changed the title (WIP) fix: map authentication errors to a custom error type fix: map authentication errors to a custom error type Jan 19, 2021
@sashahilton00 sashahilton00 added breaking includes a breaking change enhancement labels Jan 25, 2021
@sashahilton00
Copy link
Member

If there are no complaints/suggestions, I'll merge this.

@xou816
Copy link
Contributor Author

xou816 commented Feb 5, 2021

I guess ash is quite busy?

By the way I noticed you added a "breaking" label, but I had worked around breaking the api by introducing a new method, should I revert this and just update the existing method signature?

@ashthespy
Copy link
Member

Sorry, yep LGTM!
I'd forgo the deprecation, and just update the function signature. We are anyway breaking a few other things, might as well do it all at once.

@xou816 xou816 force-pushed the fix/map-auth-errors branch from c887491 to 71e9295 Compare February 5, 2021 13:26
@xou816
Copy link
Contributor Author

xou816 commented Feb 5, 2021

No worries, thank you for the review :) I removed the duplicated method!

@sashahilton00
Copy link
Member

I guess ash is quite busy?

By the way I noticed you added a "breaking" label, but I had worked around breaking the api by introducing a new method, should I revert this and just update the existing method signature?

As ash said, break it. Next version will be 0.2.0, probably followed by a couple of patches, then probably 0.3.0 once the tokio migration has completed along with a couple of other things.

@xou816
Copy link
Contributor Author

xou816 commented Feb 5, 2021

All good then, I force pushed my branch and removed the backward compatible method!

@sashahilton00 sashahilton00 merged commit 84ba421 into librespot-org:dev Feb 6, 2021
@xou816 xou816 mentioned this pull request Feb 18, 2021
Closed
@Johannesd3 Johannesd3 mentioned this pull request Feb 23, 2021
Merged
@LennyPenny LennyPenny mentioned this pull request Mar 8, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ashthespy ashthespy ashthespy approved these changes

Assignees
No one assigned
Labels
breaking includes a breaking change enhancement
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

librespot panics! when authentication fails
3 participants
@xou816 @sashahilton00 @ashthespy