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

Cargo lib bin split #4

Merged
merged 13 commits into from
Dec 16, 2023
Merged

Conversation

dsheets
Copy link
Contributor

@dsheets dsheets commented Dec 10, 2023

Hi! I wrote this exact module a few years ago in python for a Christmas present but I'm now rewriting that present (in rust because python makes me sad) to integrate more features for another Christmas present and I came across this repo. Thanks so much for publishing it! You're saving me hours of time re-implementing.

Anyway, I would really like to have the executable and the library separate so I've split them. It builds now and the tests pass and clippy is silent.

I'm happy to collaborate to get this merged. Thanks!

@TimotheeGerber
Copy link
Owner

Hi @dsheets!

Making both a lib crate and a bin crate is something I had in mind since I started this project. So your MR is more than welcome! Thank you for taking the time to do it.

I'll have a look to your code this week and let you know if I can merge it as is or if some modifications are needed.

Key-value pairs were being POSTed (correct by Spotify's spec) for
action=addUser but they were in the query string rather than in the
x-www-form-urlencoded body. This caused librespot-java 1.6.3 to refuse
connection as it could not find the action type (addUser) because it
wasn't in the body. The official Spotify client uses the body of the
POST. The rust librespot application must be lenient in this regard.

The added urlencoding dependency was already a transitive dependency
of the minreq create with 'urlencoding' feature.
@dsheets
Copy link
Contributor Author

dsheets commented Dec 12, 2023

Great, thanks! I fixed a bug I found when trying to connect to librespot-java and I started extracting more functionality from the binary into the library.

@dsheets dsheets force-pushed the cargo-lib-bin-split branch from 185fe2c to 656fde7 Compare December 12, 2023 21:27
Also adds log and env_logger dependencies so the broken out function
can report its status.
Also clean up a few dependencies that the binary had that were
transitive dependencies only
Also make the type serde Serializable because one may wish to store
device info to examine later
@dsheets dsheets force-pushed the cargo-lib-bin-split branch from d881596 to 8781674 Compare December 12, 2023 21:53
Copy link
Owner

@TimotheeGerber TimotheeGerber left a comment

Choose a reason for hiding this comment

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

Wow! You did an incredible work. Not only you separate the project in a lib and a bin crates, but you also add proper error handling, cleaned/updated dependencies and added some new features. Thanks a lot for this huge contribution!

I tested it on my local network and it still works as previously.

I only have a change request about the clientID. I can not test it because I don't have an official Spotify hardware, but I'm pretty sure it is important. Once corrected, I will merge your contribution.

The small question about activeUser is not blocking. I am just curious.

lib/src/net.rs Outdated Show resolved Hide resolved
lib/src/net.rs Show resolved Hide resolved
@TimotheeGerber TimotheeGerber merged commit 6a98966 into TimotheeGerber:main Dec 16, 2023
@TimotheeGerber
Copy link
Owner

Merged 🎉

Once again, thank you for this major contribution. Good luck with your Christmas presents!

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.

2 participants