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

Add Google Music plugin #2586

Merged
merged 5 commits into from
Jun 14, 2017
Merged

Add Google Music plugin #2586

merged 5 commits into from
Jun 14, 2017

Conversation

tigranl
Copy link
Contributor

@tigranl tigranl commented Jun 5, 2017

Here is @owcz requested implementation of Google Music plugin (#2553). Plugin is able to search and upload tracks to GM library. Options and arguments are specified in the doc file.

@tigranl
Copy link
Contributor Author

tigranl commented Jun 5, 2017

It seems like Travis has a problem running test for docs:
ERROR: InvocationError: '/home/travis/build/beetbox/beets/.tox/py34-flake8/bin/flake8 --min-version 3.4 beets beetsplug beet test setup.py docs'

Copy link
Member

@sampsyo sampsyo left a comment

Choose a reason for hiding this comment

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

Cool! This seems neat. Thanks for opening the pull request to revive the plugin! This seems like it should be useful for people in the Google ecosystem. Here are a few suggestions and an explanation for the Travis complaint.

try:
mobile.login(email, password, Mobileclient.FROM_MAC_ADDRESS)
files = mobile.get_all_songs()
except:
Copy link
Member

Choose a reason for hiding this comment

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

Travis is complaining about this bare except:. In general, it's nicer to identify exactly which exceptions can be raised. Are there exception classes in gmusicapi that we should be paying attention to?

Gmusic Plugin
=============

This plugin allows you to manage your Google Play Music library with beets.
Copy link
Member

Choose a reason for hiding this comment

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

The docs should probably explain that you need to install the gmusicapi module from pip to use this plugin, right? (See other plugin documentation for examples—e.g., lyrics or lastgenre.)

if os.path.isfile(gmusicapi.clients.OAUTH_FILEPATH):
m.login()
else:
m.perform_oauth()
Copy link
Member

Choose a reason for hiding this comment

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

Instead of using a global variable for the Musicmanager, is it possible to use a field on the Gmusic class? That can make debugging much easier.


def search(self, lib, opts, args):
email = config['gmusic']['email'].as_str()
password = config['gmusic']['password'].as_str()
Copy link
Member

Choose a reason for hiding this comment

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

It would be a good idea to mark the password field (and possibly the email field) as redacted. See other plugins that use passwords in the config file for how to do this.

email: email
password: password

It's not necessary if you only upload files.
Copy link
Member

Choose a reason for hiding this comment

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

Maybe move this disclaimer to before the configuration listing?

Remove Musicmanager global variable.
Update gmusic.rst
@tigranl
Copy link
Contributor Author

tigranl commented Jun 11, 2017

@sampsyo fixed it.

@sampsyo
Copy link
Member

sampsyo commented Jun 12, 2017

Cool; thanks! The only other thing Travis is complaining about is that we need to list the new document in the main plugins/index.rst file, which has a directory of all the plugins. The docs themselves could use a little bit of attention to regarding the installation of dependencies.

@owcz
Copy link
Contributor

owcz commented Jun 12, 2017

Is there any Google Music API to use for login credentials instead of storing it locally?

@tigranl
Copy link
Contributor Author

tigranl commented Jun 12, 2017

@sampsyo should I put it under 'Miscellaneous' section?

@tigranl
Copy link
Contributor Author

tigranl commented Jun 12, 2017

@owcz unfortunately no.

@sampsyo
Copy link
Member

sampsyo commented Jun 12, 2017

Yes, sounds good!

@sampsyo sampsyo merged commit 2d7962c into beetbox:master Jun 14, 2017
sampsyo added a commit that referenced this pull request Jun 14, 2017
sampsyo added a commit that referenced this pull request Jun 14, 2017
sampsyo added a commit that referenced this pull request Jun 14, 2017
@sampsyo
Copy link
Member

sampsyo commented Jun 14, 2017

Thank you again for your quick work on this, @tigranl! This looks awesome again. I've merged, refined the docs a bit, and added a changelog.

@tigranl tigranl deleted the gmusic branch August 8, 2017 14:25
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.

3 participants