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

merge cli and config flags #283

Merged
merged 10 commits into from
Sep 15, 2019
Merged

merge cli and config flags #283

merged 10 commits into from
Sep 15, 2019

Conversation

mainrs
Copy link
Member

@mainrs mainrs commented Sep 8, 2019

This PR merges the CLI and config options into one common struct to prevent different naming schemes. As far as I can tell, the change is fully backwards compatible thanks to clap's and serde's alias keywords.

I basically only changed the parsing logic, the outer interface is still the same (SpotifydConfig).

CLI arguments take priority over config arguments. They are merged together during load. In theory, different file formats other than INI could be supported but I opted out of this one for now. I had to re-declare librespot enums like Bitrate to add help text to them.

The following keys have changed (the old keys could be removed in a breaking change later on):

Old value New Value
volume-control volume_controller
volume-normalisation volume_normalisation
use-keyring use_keyring
normalisation-pregain normalisation_pregain
onevent on-song-change-hook

The idea behind the last one was to add more unified hooks later on (like on-login-hook or on-device-connected-hook, meaning that people can create more custom scripts to show notifications or do other stuff).

I still need to clean up the code as I can re-arrange a lot of stuff to make it more readable.
Creating the PR so you guys know that someone is working on it :)

The following things still need to be done:

  • cleanup dependencies
  • re-format code
  • re-structure cli and config structs (embedding them the other way around makes more sense)
  • implement zeroconf and http

The PR will, after completion, affect the following issues:
Closes #233.
Closes #265.

Sven Lechner added 6 commits September 9, 2019 21:13
This commit switched up the nesting order of the different config structs, allowing some code be to cleaned up. Additionally, a logging entry has been added to log the final used config before launching the daemon. This will make debugging and helping out on problems a little bit easier. Special care has been taken to not log the user's password.
This commit adds support for the zeroconf_port provided by librespot. It is now possible to pin down the port for the Spotify Connect discovery service.
@mainrs mainrs requested a review from SimonTeixidor September 9, 2019 22:47
@mainrs
Copy link
Member Author

mainrs commented Sep 9, 2019

I think I got everything done. Would be nice if you could take a look and give an OK @simonpersson :)
The flags and the config entries should be completely backwards compatible.

Ups, just noticed that I actually do not check for the spotifyd and global headers in the config file 🤐

@mainrs mainrs changed the title [WIP] merge cli and config flags merge cli and config flags Sep 10, 2019
Sven Lechner added 3 commits September 15, 2019 11:37
This commit adds the serde default flag to the FileConfig. This allows the two boolean flags to be optional instead of required.
@mainrs mainrs merged commit bd5ed24 into Spotifyd:master Sep 15, 2019
@SimonTeixidor
Copy link
Member

Thank you for this! I have been away from my computer for some time, so I wasn't able to review it for you. Looks like a great improvement :)

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.

Support for zeroconf flag in librespot Unify config file key names
2 participants