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

Move decoder to playback crate #692

Merged

Conversation

Johannesd3
Copy link
Contributor

Here's a PR that suggests moving the decoders and @roderickvd's new convert module to librespot-playback, which is the only crate that uses this stuff. Decoding is not Spotify-specific, and librespot should not expose thin wrappers for Vorbis decoders IMO.

@Johannesd3 Johannesd3 force-pushed the move-decoder-to-playback branch from e540d56 to d5a7bd1 Compare April 13, 2021 09:50
@Johannesd3 Johannesd3 mentioned this pull request Apr 20, 2021
@Johannesd3 Johannesd3 added the breaking includes a breaking change label May 1, 2021
@Johannesd3 Johannesd3 force-pushed the move-decoder-to-playback branch from d5a7bd1 to a996aad Compare May 5, 2021 10:06
@roderickvd
Copy link
Member

Would be nice to get this in, so I can rework #694 and #685 on top of it.

@Johannesd3 Johannesd3 force-pushed the move-decoder-to-playback branch from a996aad to 4566480 Compare May 8, 2021 21:41
@Johannesd3 Johannesd3 force-pushed the move-decoder-to-playback branch from 4566480 to 555274b Compare May 11, 2021 18:37
@sashahilton00 sashahilton00 merged commit b7685e3 into librespot-org:dev May 11, 2021
@roderickvd
Copy link
Member

Thanks for this. I'll be away from my development setup a few days and get back to merging with my other PR's later.

@Johannesd3 what did you think of my changelog wording proposal? I suggest looking at it from librespot as a whole instead of its separate crates. Which respectively would make it a change or removal (but then at the same time an addition).

Also thinking if it'd be a good idea to give breaking changes an extra tag? For example suffixing it with "(breaking)" at the end of the line. Where breaking is from an API-perspective (some changes may change behavior but not break linking so it might be good to be clear on what's breaking and what isn't).

@sashahilton00
Copy link
Member

What I normally do for commits on projects in production is use the conventional commits specification, combined with automated release management. I'm not sure it would work for this project in its current state, and it forces contributors to conform to a commit message specification of the format type(scope): message, something that I'm not too keen to suggest forcing on people as it irritates me when the shoe is on the other foot. We should give some consideration to whether once stable we should have a more rigid specification that allows releases to be automatically generated from commit messages, or go for a different solution that doesn't rely on a specific format. The latter is probably more user friendly, given that this software doesn't need to be 'production grade'

@Johannesd3
Copy link
Contributor Author

@sashahilton00 The site where I had the changelog scheme from doesn't think commit changelog are a good idea: https://keepachangelog.com/en/1.0.0/#log-diffs

@roderickvd Of course we could refine our changelog whenever we want. I think it's just important to add all changes directly to avoid forgetting some.

@sashahilton00
Copy link
Member

@Johannesd3 in general I agree, it isn't. you usually just get a soup of random comments, to mitigate this one has to be pretty strict about using feature branches and squashing commits. That's why one uses a spec such as the one mentioned above, so that a release tool such as semantic-release can produce a changelog with context, separating out the bug fixes, added features, performance improvements and breaking changes. It's great when developing microservices, I'm less sure of it's utility for developing libraries such as librespot. Thought I'd just mention it though as an example of how one might improve the release process of librespot once it's stable.

@Johannesd3 Johannesd3 deleted the move-decoder-to-playback branch May 19, 2021 19:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking includes a breaking change
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants