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

gmusic plugin fixes and additions #3004

Merged
merged 4 commits into from
Aug 15, 2018

Conversation

michaelmob
Copy link
Contributor

Currently, if the gmusic plugin is used, any command ran with beets will run the gmusicapi oauth authentication even if it has nothing related to gmusic.

Example, using command "ls" triggers the auth (every command does, including "web"):
bad gmusic

Fixed example, "ls" command does not trigger auth, but using one of the gmusic commands does:
bad gmusic

@michaelmob michaelmob changed the title gmusic plugin fix: only authenticate when needed gmusic plugin fixes and additions Aug 15, 2018
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.

Looks good! I just have two minor comments in this review.

This option only needs to be set if you receive an `InvalidDeviceId`
exception. Below the exception will be a list of valid device IDs.
Default: none.
- **oauth_filepath**: Filepath for oauth credentials file.
Default: none.
Copy link
Member

Choose a reason for hiding this comment

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

Maybe it’s worth explaining that this doesn’t mean that there is no OAuth file path by default; its location is just determined by the library.

Also, since it’s definitely a path, I recommend simplifying the name to either oauth_file or oauth_path.

# Checks for OAuth2 credentials,
# if they don't exist - performs authorization
oauth_filepath = (self.config['oauth_filepath'].as_str()
or gmusicapi.clients.OAUTH_FILEPATH)
Copy link
Member

Choose a reason for hiding this comment

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

One possible simplification would be to put gmusicapi.clients.OAUTH_FILEPATH into the defaults dictionary for the plugin’s configuration. Then it wouldn’t be necessary to have an explicit fallback here.

@sampsyo
Copy link
Member

sampsyo commented Aug 15, 2018

Fantastic; thanks!

@sampsyo sampsyo merged commit 890ba85 into beetbox:master Aug 15, 2018
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