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: add --uri-format option #5048

Merged

Conversation

mgoltzsche
Copy link
Contributor

@mgoltzsche mgoltzsche commented Dec 15, 2023

Beets' web API already allows remote players to access audio files but it doesn't provide a way to expose the playlists defined using the smartplaylist plugin.
This PR makes the smartplaylist plugin provides an option to generate ID-based item URIs/URLs instead of paths.
Once playlists are generated this way, they can be served using a regular HTTP server such as nginx or copied to clients/players.

To provide sufficient flexibility for various ways of integrating beets remotely (e.g. beets API, beets API with context path, AURA API, mopidy resource URI, etc), the new option has been defined as a template with an $id placeholder (assuming each remote integration requires a different path schema but they all rely on using the beets item id as identifier/path segment).

To prevent local path-related plugin configuration from leaking into a HTTP URL-based playlist generation (invoked with CLI option in addition to the local playlists generated into another directory), setting the new option makes the plugin ignore the other path-related options prefix, relative_to, forward_slash and urlencode.

Usage examples:

  • beet splupdate --uri-format 'http://beets:8337/item/$id/file' (for beets web API)
  • beet splupdate --uri-format 'http://beets:8337/aura/tracks/$id/audio' (for AURA API)
  • beet splupdate --uri-format 'beets:library:track;$id' (for mopidy-beets)

(While it was already possible to generate playlists containing HTTP URLs previously using the prefix option, it did not allow to generate ID-based URLs pointing to the beets web API but required to expose the audio files using a web server directly and refer to them using their file system $path.)

Depends on:

Relates to:

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 smartplaylist-track-path-template branch 2 times, most recently from a7c3aea to 36f3db4 Compare December 15, 2023 03:48
@mgoltzsche mgoltzsche marked this pull request as draft December 15, 2023 03:48
@mgoltzsche mgoltzsche force-pushed the smartplaylist-track-path-template branch from 36f3db4 to b80777e Compare December 15, 2023 03:56
@mgoltzsche mgoltzsche marked this pull request as ready for review December 15, 2023 04:03
@sampsyo
Copy link
Member

sampsyo commented Dec 15, 2023

Looks good overall, and it doesn't seem likely to disrupt other users of this plugin!

Here is one small design-choice: currently, the template uses the literal string $id and a plain old str.replace to fill it in. Would it make sense to future-proof this a little by using proper Python string templates? The placeholder would be {id}, and we'd fill it in like tpl.format(id=...). Maybe that's over-engineering and we never want that level of complexity, but I just thought I'd ask. 😃

@mgoltzsche
Copy link
Contributor Author

mgoltzsche commented Dec 15, 2023

@sampsyo I am fine with making the change you suggested. However, can you please elaborate in which way your suggested change is making it more future-proof.
My thinking to pick the $id placeholder was to provide a UX that is consistent with the path templates within the configuration while at the same time I didn't want to use item.evaluate_template for this purpose yet since it supports a bunch of other placeholders that are not required yet (as far as I can see) but eventually somebody may require them one day and even have the need to add the song title as URL-encoded path segment or so which could be possible using the template functionality that is also used for the file paths. In case such a feature would be required one day, tpl.replace("$id", ...) could be replaced with item.evaluate_template() without breaking compatibility. Your suggested change is fine for the current requirements but as soon as somebody has more complex requirements, she either has to implement it within another new option or break compatibility (in absence of developing another template/expression language on top of the str.format syntax).

@mgoltzsche mgoltzsche force-pushed the smartplaylist-track-path-template branch from b80777e to 29ffebc Compare December 15, 2023 20:30
@mgoltzsche
Copy link
Contributor Author

mgoltzsche commented Dec 15, 2023

Thinking about consistency, I realize it would be more consistent to call the option --format instead of --uri-template, aligned with the beet ls command's option with the same name.
Correspondingly, the option to enable m3u8 (#5053) should be renamed from --format to --output (or so)...
WDYT?

@mgoltzsche mgoltzsche force-pushed the smartplaylist-track-path-template branch from 29ffebc to b723c5b Compare December 15, 2023 22:45
@mgoltzsche mgoltzsche changed the title smartplaylist: add --uri-template option smartplaylist: add --format option Dec 15, 2023
@mgoltzsche
Copy link
Contributor Author

mgoltzsche commented Dec 15, 2023

I adjusted both PRs correspondingly (--output=m3u|m3u8 and --format=.../$id/... options). Though, corresponding to the ls command's --format option, it is still using $id as placeholder. Do you want me to change that?

@mgoltzsche mgoltzsche force-pushed the smartplaylist-track-path-template branch from b723c5b to 3b6b6c6 Compare December 15, 2023 23:03
@mgoltzsche mgoltzsche changed the title smartplaylist: add --format option smartplaylist: add --uri-format option Dec 15, 2023
@mgoltzsche mgoltzsche marked this pull request as draft December 15, 2023 23:05
@mgoltzsche mgoltzsche force-pushed the smartplaylist-track-path-template branch from 3b6b6c6 to a0b34e0 Compare December 15, 2023 23:05
@mgoltzsche
Copy link
Contributor Author

mgoltzsche commented Dec 15, 2023

I renamed the option again: it is now called --uri-format to be more specific and leave the door open for consistent CLI evolution to template additional fields in the future, if needed.
Now the PR is really ready for re-review 😅.
Do you still want me to use {id} instead of $id as placeholder, taking the UX consistency into account?

@mgoltzsche mgoltzsche marked this pull request as ready for review December 15, 2023 23:10
Beets web API already allows remote players to access audio files but it doesn't provide a way to expose the playlists defined using the smartplaylist plugin.
Now the smartplaylist plugin provides an option to generate ID-based item URIs/URLs instead of paths.
Once playlists are generated this way, they can be served using a regular HTTP server such as nginx.

To provide sufficient flexibility for various ways of integrating beets remotely (e.g. beets API, beets API with context path, AURA API, mopidy resource URI, etc), the new option has been defined as a template with an `$id` placeholder (assuming each remote integration requires a different path schema but they all rely on using the beets item `id` as identifier/path segment).

To prevent local path-related plugin configuration from leaking into a HTTP URL-based playlist generation (invoked with CLI option in addition to the local playlists generated into another directory), setting the new option makes the plugin ignore the other path-related options `prefix`, `relative_to`, `forward_slash` and `urlencode`.

Usage examples:
* `beet splupdate --uri-format 'http://beets:8337/item/$id/file'` (for beets web API)
* `beet splupdate --uri-format 'http://beets:8337/aura/tracks/$id/audio'` (for AURA API)

(While it was already possible to generate playlists containing HTTP URLs previously using the `prefix` option, it did not allow to generate ID-based URLs pointing to the beets web API but required to expose the audio files using a web server directly and refer to them using their file system `$path`.)

Relates to beetbox#5037
@mgoltzsche mgoltzsche force-pushed the smartplaylist-track-path-template branch from a0b34e0 to 58e5b02 Compare December 16, 2023 04:38
@sampsyo
Copy link
Member

sampsyo commented Dec 16, 2023

I am fine with making the change you suggested. However, can you please elaborate in which way your suggested change is making it more future-proof.

Basically, I just meant that—if we ever want to have more complex formatting going on here than just substituting a single number with no conversion—we might want a more flexible way to specify the format. But you make a good point that, even if we did that, we might still want beets's $foo-style templating instead of Python's {foo}-style templating!

Copy link
Member

@sampsyo sampsyo left a comment

Choose a reason for hiding this comment

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

This all looks great to me!! I like the new name uri_format; it is a bit more consistent. I'll merge this now!

@sampsyo sampsyo merged commit 3165d5d into beetbox:master Dec 16, 2023
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