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

info plugin: Allow custom formatting and human-readable lengths #1737

Merged
merged 8 commits into from Nov 26, 2015

Conversation

Projects
None yet
2 participants
@diego-plan9
Member

diego-plan9 commented Nov 24, 2015

Work in progress for implementing the feature discussed on #1689, as the workflow proposed by @awesomer would be pretty much aligned with one of my long-term wishlist for my library (ie. a neat way to assist in adding obscure/missing albums to MusicBrainz without the need for extra tools), and hopefully encourage others contributing back to MusicBrainz.

These commits intend to implement the suggested initial changes on the info plugin:

A good first step, should anyone choose to accept it, would be to extend the info plugin so it can be configured to output using formatting, like the list command.

The command now accepts the standard formatting argument (via cmd.parser.add_format_option), relying on the Item.__format__ machinery for formatting. Modifications have been made so an Item (either directly retrieved from the library if using library_data_emitter, or a "fake" temporary one built from the media file is using tag_data_emitter) is available to print_data().

At the same time, it would be a good idea to format length as a time by default—nobody probably wants to see a raw number of seconds.

Makes much sense to me! Just in case the raw number of seconds is useful to someone, an argument (--raw-length) has been added to switch between raw length and human-readable length (human-readable being the default).

I'd love some feedback on the current work, and also some thoughts on the preferred way for implementing the next steps. My thoughts go along the line of extending the info plugin with a listener for the "MusicBrainz match not found" event, but I'm not sure if this functionality would be better fitted into a less generic plugin?

diego-plan9 added some commits Nov 24, 2015

info: add item format and length format arguments
* Add custom output formatting via a format string to InfoPlugin. The command
accepts a formatting string via the "-f" parameter, which is handled by its
CommonOptionsParser and applied during print_data().
* Modify the emitters in order to include an Item into the list of fields, that
is formatted according to the format string if specified.
* Add an argument to allow the user to choose if track lengths are displayed as
raw floats or using a human-readable form (mm:ss), defaulting to human-readable
form.
info: minor cleanups
* Rename filter() function to avoid warning of reserved built-in symbol.
* Remove mediafile fixture on two tests.
info: add unit tests
* Add tests for length (human/raw, library/path) and custom format.
Show outdated Hide outdated beetsplug/info.py Outdated
Show outdated Hide outdated beetsplug/info.py Outdated
@sampsyo

This comment has been minimized.

Show comment
Hide comment
@sampsyo

sampsyo Nov 25, 2015

Member

Great! The format flag is an excellent first step, and something that could be useful in other contexts too—not just for uploading to MusicBrainz.

If it's OK with you, let's merge the format flag now. But I have a suggestion for the time formatting: let's make this the default for all of beets, not just the info plugin. Specifically, in library.py, let's change the $length field from types.FLOAT to a new type (TimeType?) that has a smarter format method. That way, even beet ls -f '$length' will get the new behavior.

Of course, that makes it somewhat more complicated to revert to the old, raw number if you want that for some reason. This, however, is a broader issue also shared by other fields like $bitrate and even $track, which people occasionally wish they could print as raw values. The current workaround is to define an inline field that just echoes the underlying field. But maybe we can think of a better solution for that also.

Member

sampsyo commented Nov 25, 2015

Great! The format flag is an excellent first step, and something that could be useful in other contexts too—not just for uploading to MusicBrainz.

If it's OK with you, let's merge the format flag now. But I have a suggestion for the time formatting: let's make this the default for all of beets, not just the info plugin. Specifically, in library.py, let's change the $length field from types.FLOAT to a new type (TimeType?) that has a smarter format method. That way, even beet ls -f '$length' will get the new behavior.

Of course, that makes it somewhat more complicated to revert to the old, raw number if you want that for some reason. This, however, is a broader issue also shared by other fields like $bitrate and even $track, which people occasionally wish they could print as raw values. The current workaround is to define an inline field that just echoes the underlying field. But maybe we can think of a better solution for that also.

diego-plan9 added some commits Nov 25, 2015

info: revert human_length changes
* Remove human length changes from the plugin and the tests, as they will
eventually be handled at a higher level.
info: revert human_length changes 2
* Remove human_length parameter from print_data()
@diego-plan9

This comment has been minimized.

Show comment
Hide comment
@diego-plan9

diego-plan9 Nov 25, 2015

Member

Sure, I'd be happy to leave out the time formatting changes out in favor of implementing them later at a wider scope - as a matter of fact, just pushed some commits that remove that functionality. At a first glance, I'd prefer to leave that change to someone more experienced with the types system instead of working on it myself, but if no one steps up I'd be more than willing to give it a try.

Member

diego-plan9 commented Nov 25, 2015

Sure, I'd be happy to leave out the time formatting changes out in favor of implementing them later at a wider scope - as a matter of fact, just pushed some commits that remove that functionality. At a first glance, I'd prefer to leave that change to someone more experienced with the types system instead of working on it myself, but if no one steps up I'd be more than willing to give it a try.

diego-plan9 added some commits Nov 25, 2015

info: modify emitter output, clearer path handling
* Make emitters produce a pair (dict, Item), in order to preserve the output
at print_data (dict is used if no custom format is specified, Item otherwise).
* Simplify the handling of the paths, printed at the top of print_data. The
path key is removed from the dict entirely and fetched from the Item.
@sampsyo

This comment has been minimized.

Show comment
Hide comment
@sampsyo

sampsyo Nov 26, 2015

Member

Wonderful! This looks great, and I appreciate the simplification of the path handling too. Let's call this done!

Member

sampsyo commented Nov 26, 2015

Wonderful! This looks great, and I appreciate the simplification of the path handling too. Let's call this done!

sampsyo added a commit that referenced this pull request Nov 26, 2015

Merge pull request #1737 from diego-plan9/mbtracks
info plugin: Allow custom formatting and human-readable lengths

@sampsyo sampsyo merged commit a333777 into beetbox:master Nov 26, 2015

2 checks passed

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

@diego-plan9 diego-plan9 deleted the diego-plan9:mbtracks branch Oct 17, 2016

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