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

fix inital_volume being a string #778

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

fix inital_volume being a string #778

wants to merge 2 commits into from

Conversation

JojiiOfficial
Copy link
Member

No description provided.

@robinvd
Copy link
Contributor

robinvd commented Jan 16, 2021

unfortunately this is a breaking change, we could also change the readme to initial_volume = "90" and keep the weird beheviour but that isnt great either

@JojiiOfficial
Copy link
Member Author

That is true but we don't need to publish a release instantly. Most people will use the latest stable version and this new way of parsing the "initial_value" will probably come with other minor breaking changes in the next version.

@robinvd
Copy link
Contributor

robinvd commented Jan 16, 2021

yes that might be a good plan, the only thing just like the TOML pr is that people will copy the config from the readme, and that wont be compatible

@slondr slondr added this to the v0.4.0 milestone Jan 17, 2021
@slondr
Copy link
Member

slondr commented Jan 17, 2021

I added this to the next breaking change milestone.

We should probably decide if we want to keep the incorrect documentation until then.

@JojiiOfficial
Copy link
Member Author

We should update the documentation to work with the currently stable version and update it when we release the new version. We could probably just add a comment that with a git version of spotifyd, the value must be set differently.

@robinvd
Copy link
Contributor

robinvd commented Jan 17, 2021

Or we could point the default branch to the latest release

@JojiiOfficial
Copy link
Member Author

Yes I was thinking about this as well. Usually I'm not a fan of such a solution because it looks like nothing is developed anymore and most people see the master branch as nightly/latest version (as far as I noticed) but in such a case like this, it is probably a solution we should consider.

@slondr
Copy link
Member

slondr commented Jan 17, 2021

Or we could just hold off on merging this until we're ready to release 0.4.

@mainrs mainrs added the pinned Apply to make stale bot ignore issue/PR. label Jan 17, 2021
@mainrs
Copy link
Member

mainrs commented Jan 17, 2021

Ye, just keep the PR open and merge once v0.4 is on the horizon. It's the easiest.

@leonm1
Copy link

leonm1 commented Feb 20, 2021

I noticed this and wanted to bring up changing the default branch to point to v.0.3.0, whereby drive-by readme readers could be shown the readme for the stable release and anyone looking to compile the master branch could do so with a simple git switch master. A note could be added to the readme for anyone who doesn't immediately notice that the default branch points to a stable release.

That way, breaking changes could be included with a consistent readme in master for people who want to build the current development branch, leading to more bleeding-edge users successfully testing the development channel and finding documentation errors and bugs (especially those arising from merging multiple breaking changes all together at release time) before release.

@slondr
Copy link
Member

slondr commented Sep 29, 2022

Alternatively, do we publish the book anywhere? It would probably make more sense if we had properly versioned documentation without expecting users to realize the cargo.toml is a year out-of-date, lol

@eladyn
Copy link
Member

eladyn commented Sep 30, 2022

I'm not sure if you mean that, but the book is published here. It seems, however, that it is updated together with the master branch. Maybe we could

@rnestler
Copy link

rnestler commented Dec 4, 2023

One could change this to be an enum with String and u16 to smooth out the transition: Warn if the user provided a string but still allow it. This would allow users to smoothly change to u16 for the value and in the next release support for String can get removed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pinned Apply to make stale bot ignore issue/PR.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants