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

mbsubmit: cleanup and completion #1779

Merged
merged 8 commits into from
Jan 2, 2016
Merged

Conversation

diego-plan9
Copy link
Member

Glad to see a new release has been made!

I'm getting back to work on beets after a few days away from the computer, hopefully bringing issue #1689 to a close eventually. As hinted on the previous discussion, this pull request is intended to take care of the mbsubmit plugin cleanup, now that the underlying pieces are in place.

I have modified a bit the behaviour, making the decision of appending the "Print tracks" choice depend solely on task.rec. The default behaviour is to only append the choice to matches where the recommendation is equal or lower than Recommendation.medium, which hopefully covers the most obvious choices (albums with no matches, albums with weak-ish matches) and the original request by @awesomer, and also avoids polluting the prompt in the cases where the match is strong. A config option has been added that allows the user to modify this settings (extra-picky users might find it useful to always be able to print tracks for fixing spelling mistakes, other users might only want it on albums with no matches, etc).

Other than that, a configuration option for setting the format string has been added as well - I can't think of a case where this might come in handy currently, but maybe more creative users might find it useful.

A couple of notes:

  • currently, the plugin makes no effort of nicely formatting items that might be lacking some of the required fields. Would it be useful to add some extra checks and fall back to printing the filename (or something more advanced with the help of fromfilename, etc) in those cases?
  • there might be some problems on some combination on options: for example, if the user sets the threshold to strong, but launches the importer in non-timid mode, the prompt will not actually be displayed. Would a note on the (upcoming) documentation suffice, as handling this case probably requires some changes that seem to be a bit out of the scope of the plugin?

As usual, any comments and input are more than welcome!

* Cleanup the "mbsubmit" plugin to remove a choice ("print tracks and skip")
and make the logic depend on the strength of the Recommendation.
* Add configuration options for the Recommendation threshold that triggers
the addition of the "Print tracks" choice and for the formatting string to
be used for printing the items.
* Include link to the official-ish MusicBrainz format page on the docstring.
* Add basic unit tests for the mbsubmit plugin, covering the output of the
"Print tracks" option on albums and singletons.
'threshold': 'medium',
})

# validate and store threshold
Copy link
Member

Choose a reason for hiding this comment

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

We tend to format comments as "full sentences" in beets—starting with a capital letter and ending with a period.

@sampsyo
Copy link
Member

sampsyo commented Dec 29, 2015

Awesome; this looks perfect. Feel free to merge at will.

On those questions:

  • I'd say we shouldn't worry about missing fields. Here's my reasoning: this plugin is about situations where you already have good metadata (or better metadata than MB). So while picking up tags from filenames à la fromfilename would be nifty, it's out of scope for this initial version.
  • I see what you're saying—yeah, let's document this.

@diego-plan9
Copy link
Member Author

Thanks for the quick turnaround, and commenting on the questions - sounds like a reasonable compromise. I will work on the documentation during tomorrow and hopefully everything should be ready for a merge soon!

@diego-plan9
Copy link
Member Author

Fixed the style issues, and added some pretty basic documentation and a changelog entry. As usual, double checking the documentation would be more than welcome!

As MusicBrainz currently does not support submitting albums programmatically,
the recommended workflow is to copy the output of the ``Print tracks`` choice
and paste it into the parser that can be found by clicking on the
"`Track Parser`" button on MusicBrainz "`Tracklist`" tab.
Copy link
Member

Choose a reason for hiding this comment

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

Any particular reason for the extra backticks here? It seems like quotes might be enough. (This is officially the tiniest style issue ever!)

@sampsyo
Copy link
Member

sampsyo commented Dec 31, 2015

This looks great! I made some extremely minor suggestions in the docs, but I think this is ready—feel free to merge this whenever you're happy with it.

Woohoo! 🎉

@diego-plan9
Copy link
Member Author

Fixed the style issues (the extra ticks were for making it more evident that they were referring to the names of the button/elements on MusicBrainz site, and the choice of mb_format for the configuration was to avoid any confusion with the format flag - but I'm happy to drop both)!

I have also merged upstream/master, solving the tiny conflict on changelog.rst.

diego-plan9 added a commit that referenced this pull request Jan 2, 2016
mbsubmit: cleanup and completion
@diego-plan9 diego-plan9 merged commit 7ff9990 into beetbox:master Jan 2, 2016
@diego-plan9
Copy link
Member Author

And merged, after the checks seemed ok!

@sampsyo
Copy link
Member

sampsyo commented Jan 2, 2016

Woohoo!

aszlig added a commit to NixOS/nixpkgs that referenced this pull request Feb 3, 2016
Updates beets to version 1.3.16, which comes with new plugins
"embyupdate", "edit" and "mbsubmit". See the following URL for a
detailed upstream changelog:

http://beets.readthedocs.org/en/v1.3.16/changelog.html

The "mbsubmit" plugin isn't listed there and made it more or less
silently into the release, see beetbox/beets#1779 for the final work on
the plugin.

Tested this locally with a few queries and using the new "edit" plugin.
@diego-plan9 diego-plan9 deleted the prompthook branch October 17, 2016 15:12
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.

2 participants