Skip to content
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

Update examples #663

Merged
merged 1 commit into from
Mar 24, 2021
Merged

Conversation

ThouCheese
Copy link
Contributor

Updated the examples to run with tokio 1.0.

@Johannesd3
Copy link
Contributor

Thanks!

I'm sorry I removed the Default impl of SessionConfig. It wasn't used in the binary, so I thought it was used nowhere, and it was the only reason why librespot-core depends on uuid. Should we add it back? Another option would be the builder pattern.

@ThouCheese
Copy link
Contributor Author

I feel like the default impl is a more ergonomic, and that a builder pattern is not necessarily warranted for a struct with 4 fields.

@ThouCheese
Copy link
Contributor Author

ThouCheese commented Mar 6, 2021

I also feel like the examples could be nicer without the let username = args[1].to_owned(); lines, so if we change the Credentials::with_password to take an impl Into<String>, those can go away.

@Johannesd3 Johannesd3 mentioned this pull request Mar 8, 2021
13 tasks
core/src/config.rs Outdated Show resolved Hide resolved
@ThouCheese
Copy link
Contributor Author

I reintroduced the old Default impl, be reintroducing the dependency on uuid.

examples/play.rs Outdated Show resolved Hide resolved
@Johannesd3
Copy link
Contributor

Looks good, but there are merge conflicts. You should rebase it (and maybe squash it into 1 or 2 commits while you're at it). And then I think it's ready.

@Johannesd3
Copy link
Contributor

@ThouCheese Would you do this?

Re-add default impl to SessionConfig and make Credentials::with_password generic over Into<String>

add docs for Credential

reintroduce old Default impl for SessionConfig

use the third argument for the track-to-play rather than a testing id
@ThouCheese
Copy link
Contributor Author

Hi! Yes sorry that took so long, I had to fight a bit with git rebase (that is, learn how it works). Currently 1 commit and no conflicts.

Copy link
Contributor

@Johannesd3 Johannesd3 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great! Even better if you've learned something new.

@ashthespy ashthespy merged commit 1051f98 into librespot-org:tokio_migration Mar 24, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants