Edit plugin

[sampsyo]

Successor to #1687. This is @jmwatte's interactive editor plugin.

[sampsyo]

@jmwatte, I'm interested in trimming down the initial version of this plugin to the bare minimum. We can add more flexibility later, but I'm really interested in getting something as simple as possible for the first iteration.

Would it be OK with you if I removed some of the indirection that supports multiple methods? For now, I'll hard-wire this to be YAML-only for readability's sake.

[sampsyo]

I'm also not 100% sure about including your (rather clever) logic for reducing the display of fields with the same value (the sum option). It's clearly going to be useful eventually, but it requires a lot of logic that makes the first version a bit harder to read. Would it also be OK if I temporarily removed that for the sake of the initial merge?

[pkess]

sadly there are two more things:

1. when i add a field in the editor like this:
   onplayer: true
   the plugin sets the field onplayer to value "1". With beet modify -w onplayer:true the field is set to true as expected
2. when i am using the "-e" commandline switch i can't use "user-defined" fields like my onplayer field. It seems like only fields from the items table in db are supported

[sampsyo]

Thanks for trying this out!

1. when i am using the "-e" commandline switch i can't use "user-defined" fields like my onplayer field. It seems like only fields from the items table in db are supported

Yep, this was an oversight. I just changed the option to work for any field.

I also changed the name from --extra or -e to --field or -f, which I think is a little easier to guess.

[sampsyo]

1. when i add a field in the editor like this:
   onplayer: true
   the plugin sets the field onplayer to value "1". With beet modify -w onplayer:true the field is set to true as expected

This is another good point—the plugin is just trusting the YAML type of each value, rather than using the beets logic for parsing strings (as the modify plugin does).

This will, unfortunately, be a little bit difficult to fix. The problem is that the YAML parser sees the values not as strings but as a whole bouquet of different types. foo: 123 gives you an integer, foo: true gives you a boolean, etc. We don't get to see the raw strings.

We'll need to resort to some sort of hack to let beets do the value parsing instead of the YAML library. Or we could switch away from YAML as the serialization format.

[diego-plan9]

An observation on the scenario presented by @pkess, which I'm unsure if it is the expected behaviour: if the user-specified extra field (-f nonexistingfield) is not already present on the item, it is currently not dumped into the yaml (flatten() silently swallows it due to iterating on obj.items()). I'm wondering if the key should be added to the yaml with an empty value instead?

[diego-plan9]

As for the problem of yaml auto-magically performing type conversion, looks like a nontrivial issue indeed! However, I'm wondering if it could be alleviated to some extent by trying to make use of Flexible fields and dbcore.types, by something along the lines of:

1. Allowing the user to specify his own custom fields and their type on his config (such as via a small plugin):

custom_fields_plugin:
  onplayer: boolean

2. Make those available as that plugin's CustomFieldsPlugin.item_types.
3. Rely on the magic of dbcore.types for casting to the right type before yaml dumping / after yaml loading.

An informal and dirty hack (by appending self.__class__.item_types = {'onplayer': types.Boolean()} to EditPlugin.__init__()) seems to result in the yaml correctly displaying onplayer: true instead of onplayer: '1'), but I'm not sure if:

• this approach is sound
• it is useful with other types, and solves more problems that it might cause
• there are enough users using custom fields to justify the extra plugin

[sampsyo]

An observation on the scenario presented by @pkess, which I'm unsure if it is the expected behaviour: if the user-specified extra field (-f nonexistingfield) is not already present on the item, it is currently not dumped into the yaml (flatten() silently swallows it due to iterating on obj.items()). I'm wondering if the key should be added to the yaml with an empty value instead?

Yes, I was thinking the same thing. The only trick with that change is that we'll need the "round trip" to stay correct: if you leave the field empty in the editor, beets shouldn't add it to the item. It's not quite clear to me how to distinguish that from adding the field with an empty value.

[sampsyo]

1. Allowing the user to specify his own custom fields and their type on his config (such as via a small plugin):

Excellent idea! 😃 http://docs.beets.io/en/v1.3.15/plugins/types.html

3. Rely on the magic of dbcore.types for casting to the right type before yaml dumping / after yaml loading.

Yes, now that you mention that, it could be as easy as just dumping everything to a string and then loading it back from a string. The only problem is if someone comes along and sees:

foo: "true"

which is a string value and changes it to:

foo: false

in an attempt to flip the Boolean. Then YAML will parse this not as the string "false" but as the Boolean value False. Sadly, dbcore's type-based string parsing infrastructure can't handle this---it wants the original string.

[diego-plan9]

Excellent idea!  http://docs.beets.io/en/v1.3.15/plugins/types.html

Well, that's embarrassing 😓. I did some greping and peek at the issues to find out if that was already in place, but seems it got past my not so deep research!

Maybe we should preliminary add some comments on the documentation of the plugin, cautioning about the potential issues with custom fields?

[sampsyo]

Here's another way to make things safe: let's dump everything to strings, as planned, and then reject modifications like the example I showed (where the user inputs a non-string type). Over time, we can make this more tolerant by accepting, for example, the Boolean True as a synonym for the string "True".

[sampsyo]

OK, here's an implementation of that idea that I'm reasonably happy with. If the user messes things up and uses a non-string value, we currently just try stringifying the value and then parsing it. This could be a little smarter, but I think this should work for most cases (and should solve the original problem).

One trick here is that we need to be sure that all data types can "round-trip" for ergonomic editing. One problem I ran into immediately was with the bitrate field: it gets formatted as "128kbps" (for example), but the parser just sees this as the number 128 instead of 128000. This is problematic because there is actually some data lost here---the low bits of the bitrate number.

[sampsyo]

One way to cope with the above problem would be to special-case some types that we know are safe for YAML editing, like the numeric types or booleans. These would somehow bypass the formatting when printed. If they come back as the same type as they were written out, then they also bypass parsing; if the user changes to a different type, then we can stringify and parse as a fallback.

[sampsyo]

OK, sorry for all the noise but this latest revision should do it. Most values are stringified and parsed again, but certain "safe" types get a pass. They are still validated to ensure nothing crazy happens, though, and parsed as strings if necessary.

[diego-plan9]

Looking good! I'm wondering if it would be a good time to include some more tests focused on specific functions on the plugin, or perhaps it's better to hold off until it is included on the main branch?

[sampsyo]

Awesome! More tests are always better, but I don't have a strong opinion about whether they should come before or after merge. If it's looking reasonably complete, I think we can merge at will!

[pkess]

Hi,

i just merged master into this branch because i experienced errors with this branch and the badfiles plugin because of unicode characters

[sampsyo]

Thanks for updating the PR, @pkess.

I think this is ready enough for inclusion in the next point release. There are still more things we could do, of course, but no need to hold it back any more. Merging now.