Add plugin for formatting albumtypes by edgars-supe · Pull Request #4048 · beetbox/beets

edgars-supe · 2021-09-09T19:42:19Z

Description

Added a plugin to add a configurable album template field $atypes to format albumtypes of an Album (see #4045).

Should I rename the plugin to atypes since that's the field it adds, and to reduce possible confusion with the actual field albumtypes?

To Do

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

sampsyo

Wow; nice work! This looks awesome so far. I have a few suggestions for the code and some documentation improvements for your consideration.

sampsyo · 2021-09-09T22:00:37Z

+        super(AlbumTypesPlugin, self).__init__()
+        self.album_template_fields['atypes'] = self._atypes
+
+    def _atypes(self, item: Album):


Perhaps add a docstring, like "generate a formatted string for an album's types"?

sampsyo · 2021-09-09T22:01:06Z

+        self.config.add({
+            'types': [],
+            'ignore_va': [],
+            'brackets': '[]'
+        })


This stuff should go in __init__; otherwise, it will try to set the defaults every time we use the $atypes field.

Good point!

Would it not be better to extract the config values (next lines after this) in __init__() as well, so it's not done every time _atypesis called? I see both approaches in other plugins, so I suppose it's not a big concern, but converting config to those structs isn't exactly free.

That is less of a big deal, IMO, and it's fine to do it either way. The reason is that config.add(...) actually modifies config: it inserts a new "layer" with the default configuration values. So doing this every time actually adds multiple copies of that data. In contrast, get and as_* calls are read-only, so it doesn't matter as much where they appear.

sampsyo · 2021-09-09T22:02:23Z

+        is_va = item.mb_albumartistid == VARIOUS_ARTISTS_ID
+        for type in types:
+            if type[0] in albumtypes and type[1]:
+                if not is_va or (not type[0] in ignore_va and is_va):


not type[0] in ignore_va is probably more clearly written type[0] not in ignore_va.

sampsyo · 2021-09-09T22:03:43Z

+        for type in types:
+            if type[0] in albumtypes and type[1]:
+                if not is_va or (not type[0] in ignore_va and is_va):
+                    res += bracket_l + type[1] + bracket_r


This might be a little easier to read as a PEP 498 format/template string? Like: f'{bracket_l}{type[1]}{bracket_r}'

sampsyo · 2021-09-09T22:04:25Z

+such as "Album", "EP", "Single", etc. List of available type can be found
+`here`_.


Maybe for this last link, I'd word it as: "For the list of available album types, see the MusicBrainz documentation." (with the last two words linked)

sampsyo · 2021-09-09T22:04:54Z

+(see :ref:`using-plugins`). Then, add ``$atypes`` to your path formats as
+desired.


"The plugin defines a new field $atypes, which you can use in your path formats or elsewhere."

sampsyo · 2021-09-09T22:06:15Z

+- **types**: An ordered list of album type to format mappings. The order of the
+  mappings determines their order in the output. If a mapping is missing or
+  blank, it will not be in the output.
+  Default: ``[]``.


Maybe just say "Default: None." ([] implies it's a list, not a mapping.)

But also… should the default be nothing? Does that mean that $atypes will always be empty by default? Perhaps this should come with a default mapping, like the one shown below, so the plugin is useful "out of the box"?

That's a good idea, I think. I've set the default values to the example config and adjusted the docs accordingly.

sampsyo · 2021-09-09T22:07:04Z

+- **ignore_va**: A list of types that should not be output for Various Artists
+  albums. Useful for not adding redundant information - various artist albums
+  are often compilations.
+  Default: ``[]``.


Again, perhaps the default here should be what's shown below—i.e., "compilation"? It seems like a sensible default for most people.

sampsyo · 2021-09-09T22:07:26Z

+        albumtype:soundtrack Various Artists/$album ($year)$atypes)/...
+        comp: Various Artists/$album ($year)$atypes/...
+
+Example outputs::


"This configuration generates paths that look like this, for example::"

sampsyo · 2021-09-09T22:07:35Z

+    Pink Flow/(1995)(Live) p·u·l·s·e
+    Various Artists/20th Century Lullabies (1999)
+    Various Artists/Ocean's Eleven (2001)(OST)
+


Probably no need for this extra line.

edgars-supe · 2021-09-10T07:28:48Z

I've done the improvements you mentioned, added sane defaults, etc.

The comment about formatting output got me wondering, though: would it not be better to provide prefix, separator and suffix config values? That way with, say, [, , , ] you could have [EP, Remix], if that's what you prefer. Or (, )(, ) to have the current output.

Also, still wondering if I should rename the plugin to atypes, because that's the template field it adds and to not confuse it with the actual field on Album. Thoughts?

sampsyo

Awesome! This looks great; I think it's basically good to go.

About the additional configuration values: I don't have a strong feeling either way! I definitely can envision people wanting a formatting like [EP, Remix]. But it's also totally fine to merge a minimal version of the plugin that fits your needs and to add more features later, "on demand," as people actually want them. (To philosophize for a moment, that strategy can be higher-overhead but lead to outcomes that more closely match what users want.) May I leave that decision up to you?

I usually have opinions about naming, but I am also finding myself without a strong feeling about the plugin name… I definitely see the argument for naming it the same as the field it creates, but the current name helps describe what it does. So again, I'm perfectly happy either way!

sampsyo · 2021-09-10T11:03:16Z

+  are often compilations.
+- **bracket**: Defines the brackets to enclose each album type in the output.
+
+The default configuration looks like this: ::


Suggested change

The default configuration looks like this: ::

The default configuration looks like this::

(I believe this is equivalent.)

edgars-supe · 2021-09-10T11:55:31Z

Either way is fine with me, so I'll just leave things as they are currently. I don't personally have a need to do [EP, Remix]. :D
I'll perhaps work on that at some point, but if anyone expresses a strong need for such a feature, I can do it, no problem.

I adjusted the docs based on your comment, so if there's nothing more, it's ready to be merged. :)

sampsyo · 2021-09-10T20:47:30Z

Excellent! Nice work on this!! 🎉 🚀

edgars-supe added 5 commits September 9, 2021 23:05

Add albumtypes plugin

bee3588

Add tests for albumtypes plugin

6fdc505

Add documentation for albumtypes plugin

8991367

Add albumtypes plugin to changelog

0a4aa40

Add albumtypes to plugin index

c59a22e

sampsyo requested changes Sep 9, 2021

View reviewed changes

edgars-supe added 2 commits September 10, 2021 10:23

Improve albumtypes plugin code

131befa

Improve albumtypes plugin documentation

c5764df

sampsyo approved these changes Sep 10, 2021

View reviewed changes

Fill default config for albumtypes plugin

d40f0c8

sampsyo merged commit e1d41c2 into beetbox:master Sep 10, 2021

edgars-supe deleted the albumtypes-plugin branch September 10, 2021 20:49

arogl added a commit to arogl/beets that referenced this pull request Sep 26, 2021

Commits beetbox#4048 & beetbox#4050

8205b90

		such as "Album", "EP", "Single", etc. List of available type can be found
		`here`_.

		(see :ref:`using-plugins`). Then, add ``$atypes`` to your path formats as
		desired.

	The default configuration looks like this: ::
	The default configuration looks like this::

Uh oh!

Conversation

edgars-supe commented Sep 9, 2021

Description

To Do

Uh oh!

sampsyo left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

edgars-supe commented Sep 10, 2021

Uh oh!

sampsyo left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

edgars-supe commented Sep 10, 2021

Uh oh!

sampsyo commented Sep 10, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants