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

Edit plugin #1706

Merged
merged 82 commits into from Dec 13, 2015

Conversation

Projects
None yet
4 participants
@sampsyo
Member

sampsyo commented Nov 7, 2015

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

jmwatte added some commits Nov 2, 2015

Update changelog.rst
added yamleditor(a new plugin) entry
Update yamleditor.rst
polishing
Update yamleditor.rst
more polishing
Update yamleditor.rst
polishing
Update yamleditor.rst
polishing
Update yamleditor.py
polishing
Update yamleditor.rst
polishing
Update yamleditor.rst
polishing
Update edit.rst
polishing
Update edit.rst
polishing
Update changelog.rst
change name from yamleditor to edit
Update index.rst
added the edit plugin

This was referenced Nov 7, 2015

@sampsyo

This comment has been minimized.

Show comment
Hide comment
@sampsyo

sampsyo Nov 7, 2015

Member

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

Member

sampsyo commented Nov 7, 2015

@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

This comment has been minimized.

Show comment
Hide comment
@sampsyo

sampsyo Nov 7, 2015

Member

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?

Member

sampsyo commented Nov 7, 2015

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

This comment has been minimized.

Show comment
Hide comment
@pkess

pkess Nov 17, 2015

Collaborator

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
Collaborator

pkess commented Nov 17, 2015

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 added some commits Nov 17, 2015

--extra option can use any field
Not just the built-in fields.
@sampsyo

This comment has been minimized.

Show comment
Hide comment
@sampsyo

sampsyo Nov 17, 2015

Member

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.

Member

sampsyo commented Nov 17, 2015

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

This comment has been minimized.

Show comment
Hide comment
@sampsyo

sampsyo Nov 17, 2015

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

Member

sampsyo commented Nov 17, 2015

  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

This comment has been minimized.

Show comment
Hide comment
@diego-plan9

diego-plan9 Nov 18, 2015

Member

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?

Member

diego-plan9 commented Nov 18, 2015

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

This comment has been minimized.

Show comment
Hide comment
@diego-plan9

diego-plan9 Nov 18, 2015

Member

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
  1. Make those available as that plugin's CustomFieldsPlugin.item_types.
  2. 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
Member

diego-plan9 commented Nov 18, 2015

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
  1. Make those available as that plugin's CustomFieldsPlugin.item_types.
  2. 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

This comment has been minimized.

Show comment
Hide comment
@sampsyo

sampsyo Nov 18, 2015

Member

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.

Member

sampsyo commented Nov 18, 2015

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

This comment has been minimized.

Show comment
Hide comment
@sampsyo

sampsyo Nov 18, 2015

Member
  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

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

Member

sampsyo commented Nov 18, 2015

  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

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

This comment has been minimized.

Show comment
Hide comment
@diego-plan9

diego-plan9 Nov 19, 2015

Member

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?

Member

diego-plan9 commented Nov 19, 2015

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

This comment has been minimized.

Show comment
Hide comment
@sampsyo

sampsyo Nov 19, 2015

Member

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

Member

sampsyo commented Nov 19, 2015

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 added some commits Nov 19, 2015

Use type-based formatting and parsing
All editable values are now strings and are parsed from strings. This closely
matches the behavior of the `modify` command, for example.
Catch unexpected YAML type
We need a sequence of dictionaries back; validate this assumption.
Convert YAML keys and values back to strings
I hadn't quite realized before that the user could also change the *keys* to
be non-strings too! This also prevents against that by just reinterpreting
everything as strings.
@sampsyo

This comment has been minimized.

Show comment
Hide comment
@sampsyo

sampsyo Nov 19, 2015

Member

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.

Member

sampsyo commented Nov 19, 2015

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

This comment has been minimized.

Show comment
Hide comment
@sampsyo

sampsyo Nov 19, 2015

Member

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.

Member

sampsyo commented Nov 19, 2015

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 added some commits Nov 20, 2015

Pass through certain "safe" types to YAML
This avoids some round-tripping problems with types (such as ScaledInt) that
are not represented in strings with 100% fidelity. It also makes the syntax
nicer when editing numbers and booleans: they no longer appear to be
needlessly surrounded by quotes in the YAML.
@sampsyo

This comment has been minimized.

Show comment
Hide comment
@sampsyo

sampsyo Nov 20, 2015

Member

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.

Member

sampsyo commented Nov 20, 2015

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

This comment has been minimized.

Show comment
Hide comment
@diego-plan9

diego-plan9 Nov 23, 2015

Member

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?

Member

diego-plan9 commented Nov 23, 2015

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

This comment has been minimized.

Show comment
Hide comment
@sampsyo

sampsyo Nov 23, 2015

Member

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!

Member

sampsyo commented Nov 23, 2015

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

This comment has been minimized.

Show comment
Hide comment
@pkess

pkess Dec 2, 2015

Collaborator

Hi,

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

Collaborator

pkess commented Dec 2, 2015

Hi,

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

@sampsyo

This comment has been minimized.

Show comment
Hide comment
@sampsyo

sampsyo Dec 13, 2015

Member

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.

Member

sampsyo commented Dec 13, 2015

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.

@sampsyo sampsyo merged commit f1f1288 into master Dec 13, 2015

3 checks passed

continuous-integration/appveyor/branch AppVeyor build succeeded
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

sampsyo added a commit that referenced this pull request Dec 13, 2015

@sampsyo sampsyo referenced this pull request Dec 15, 2015

Closed

Visual tag editor #164

@sampsyo sampsyo deleted the editor branch Jan 27, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment