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

smartplaylist: allow exporting item fields, rename m3u8 output to extm3u #5121

Merged
merged 5 commits into from
Apr 20, 2024

Conversation

mgoltzsche
Copy link
Contributor

@mgoltzsche mgoltzsche commented Feb 24, 2024

Description

Allow generating extm3u playlists so that they contain additional item fields such as the id.

The feature is required by the webm3u plugin (M3U server) to transform playlists using a request based item URI template which may require additional fields such as the id, e.g. beets:library:track;$id.

To Do

  • Documentation. (If you've added a new command-line flag, for example, find the appropriate page under docs/ to describe it.)
  • Changelog. (Add an entry to docs/changelog.rst to the bottom of one of the lists near the top of the document.)
  • Tests. (Very much encouraged but not strictly required.)

@mgoltzsche mgoltzsche force-pushed the generate-playlist-item-attributes branch 4 times, most recently from 63840af to 797de85 Compare February 24, 2024 03:42
@mgoltzsche mgoltzsche marked this pull request as draft February 24, 2024 03:42
@mgoltzsche mgoltzsche force-pushed the generate-playlist-item-attributes branch 4 times, most recently from e45c4f0 to a780bfb Compare February 24, 2024 04:03
@mgoltzsche mgoltzsche marked this pull request as ready for review February 24, 2024 04:52
@Serene-Arc
Copy link
Contributor

Thank you for this PR! The code seems fine but might I ask that you add some more to the docs? Perhaps the keys to the attributes most likely to be used and an explanation for when they might be required, such as the web service you specified.

@mgoltzsche mgoltzsche marked this pull request as draft March 5, 2024 00:11
@JOJ0
Copy link
Member

JOJ0 commented Mar 7, 2024

Hi @mgoltzsche, great addition to smartplaylist and very interesting plugin you wrote. Please also add a link to your plugin's repo to this section of the docs when you are coming back to working on this PR: https://beets.readthedocs.io/en/latest/plugins/index.html#interoperability
Just including the docs change within this PR is fine in my opinion.

@mgoltzsche mgoltzsche changed the title smartplaylist: allow exporting item attributes smartplaylist: allow exporting item fields Mar 13, 2024
@mgoltzsche
Copy link
Contributor Author

mgoltzsche commented Mar 13, 2024

I rebased the PR now to resolve the merge conflict, added an entry to the interoperability section of the plugins/index.rst, added more info to the smartplaylist.rst.
Also, I changed the option name from attributes to fields because it is shorter and more aligned with the existing CLI terminology (beet fields).
Unfortunately, the GitHub data of the PR seems to be inconsistent since it doesn't show my force-pushed commit 😞....

@mgoltzsche mgoltzsche marked this pull request as ready for review March 13, 2024 01:19
@mgoltzsche mgoltzsche marked this pull request as draft March 13, 2024 01:35
@mgoltzsche
Copy link
Contributor Author

Okay, as described here re-selecting beetbox:master as base branch within the GitHub GUI updates the PR and triggers CI but now the diff also includes all other commits on the master branch since this PR was created 😞. However, my changes are still within a single commit which you could re-review.

@mgoltzsche mgoltzsche marked this pull request as ready for review March 13, 2024 01:44
@mgoltzsche mgoltzsche force-pushed the generate-playlist-item-attributes branch 8 times, most recently from 1b1db0a to 352c3e2 Compare March 13, 2024 02:43
@mgoltzsche mgoltzsche requested a review from Serene-Arc March 13, 2024 23:46
@mgoltzsche mgoltzsche marked this pull request as ready for review March 21, 2024 00:23
@mgoltzsche
Copy link
Contributor Author

mgoltzsche commented Mar 29, 2024

@JOJ0 are you okay with having the webm3u plugin link within the "Other plugins" section of the documentation as opposed to putting it in the interoperability section? Otherwise, do you think the webrouter plugin should also be listed in the interoperability documentation section?

@JOJ0
Copy link
Member

JOJ0 commented Apr 1, 2024

Ah yes please put everything into the Other Plugins section. Seems like we file all of externally hosted plugins there. My suggestion here was wrong. I'm sorry: #5121 (comment)

Allow generating extm3u playlists so that they contain additional item fields such as the `id`.

The feature is required by the mgoltzsche/beets-webm3u plugin (M3U server) to transform playlists using a request based item URI template which may require additional fields such as the `id`, e.g. `beets:library:track;$id`.
Also, moves the webm3u plugin link from tne interoperability section to the "Other Plugins" section of the documentation.
@mgoltzsche mgoltzsche force-pushed the generate-playlist-item-attributes branch from 291db57 to 3c6309f Compare April 13, 2024 02:55
@mgoltzsche
Copy link
Contributor Author

Rebased to resolve merge conflict within changelog.

Copy link
Member

@JOJ0 JOJ0 left a comment

Choose a reason for hiding this comment

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

Sorry for the massive delay in reviewing, I wanted to do this earlier. Code is fine, there is only one thing that I suggest to improve: Please add one or two example lines to the docs. How will a playlist entry look like with a specific config?

self.assertEqual(
content,
b"#EXTM3U\n"
+ b'#EXTINF:300 id="456" genre="Fake Genre",Fake Artist - fake Title\n'
Copy link
Member

Choose a reason for hiding this comment

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

Something like this: Add a config example and the resulting line on the playlist file to the docs.

@mgoltzsche
Copy link
Contributor Author

Okay, I added an example configuration and extm3u playlist to the documentation now. Please re-review!

@mgoltzsche mgoltzsche force-pushed the generate-playlist-item-attributes branch 2 times, most recently from 245361a to dc0022e Compare April 19, 2024 22:34
@mgoltzsche mgoltzsche marked this pull request as draft April 19, 2024 22:34
@mgoltzsche mgoltzsche force-pushed the generate-playlist-item-attributes branch 8 times, most recently from c9d39f7 to fba30fc Compare April 19, 2024 22:49
@mgoltzsche mgoltzsche marked this pull request as ready for review April 19, 2024 22:59
@mgoltzsche
Copy link
Contributor Author

mgoltzsche commented Apr 19, 2024

After some problems with the docs linter now the PR is ready for review again.
Also, I made the documentation mention the autogenre plugin.

@mgoltzsche mgoltzsche force-pushed the generate-playlist-item-attributes branch from 2746074 to 2e9308f Compare April 20, 2024 03:33
@JOJ0 JOJ0 merged commit d510177 into beetbox:master Apr 20, 2024
11 checks passed
@JOJ0
Copy link
Member

JOJ0 commented Apr 20, 2024

BTW I like the tiny PlaylistItem class, something like that could also help with my never finished project of providing a general m3u handling solution in beets. Currently it's rather unspectacular, it has save/load functionality only: https://github.com/beetbox/beets/blob/master/beets/util/m3u.py

oops oof-topic alert :-)

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