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

Implement multi-value genres tag #5426

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

ibrokemypie
Copy link

@ibrokemypie ibrokemypie commented Sep 19, 2024

Description

Resurrects #4751. Rebased just the genre field changes on to master.

"Adds support for setting the genre tag to be either a single or multi."

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.)

Copy link

Thank you for the PR! The changelog has not been updated, so here is a friendly reminder to check if you need to add an entry.

@ibrokemypie ibrokemypie force-pushed the feat/multiple-genre-support branch from 4b84406 to 7ef0726 Compare September 20, 2024 01:45
@ibrokemypie ibrokemypie changed the title Implement multi-tag genres field Implement multi-value genres tag Sep 20, 2024
test/test_library.py Outdated Show resolved Hide resolved
@kergoth
Copy link
Contributor

kergoth commented Sep 21, 2024 via email

test/test_library.py Outdated Show resolved Hide resolved
test/test_library.py Outdated Show resolved Hide resolved
@@ -708,13 +708,13 @@ def test_if_def_false_complete(self):
self._assert_dest(b"/base/not_played")

def test_first(self):
Copy link
Member

Choose a reason for hiding this comment

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

Could you try and test this %first{} function with the new genres field, to show how the user may use it to extract the first genre?

Copy link
Author

Choose a reason for hiding this comment

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

Currently the only way to use %first with these MULTI_VALUE_DSV fields is this (for the record, that doesn't even work for me and I have to do %first{%first{$albumartists,1,0,␀},1,0,\\} in my own config) which I think is pretty awful, not documented anywhere and not even confirmed as the intended method.

Without it being confirmed that that is the intended method of use, or a future improvement to the ergonomics, I wouldn't want to encode this in a test as the expected usage. This PR just brings up the genres field to the same state as the artists field, and any work beyond that is out of scope.

@ibrokemypie ibrokemypie force-pushed the feat/multiple-genre-support branch from 7ef0726 to d4175f7 Compare September 22, 2024 23:21
Copy link
Member

@bal-e bal-e left a comment

Choose a reason for hiding this comment

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

This is a nice and tiny PR, thanks @ibrokemypie!

@@ -20,6 +20,7 @@ New features:
* :doc:`plugins/autobpm`: Add new configuration option ``beat_track_kwargs``
which enables adjusting keyword arguments supplied to librosa's
``beat_track`` function call.
* New multi-value `genres` tag
Copy link
Member

Choose a reason for hiding this comment

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

Can you expand a bit here, maybe with an example that shows why it's useful? The point of such a tag should be made obvious to end users scrolling through.

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.

4 participants