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

Adding gmusic ability to upload on import #2998

Merged
merged 8 commits into from
Aug 10, 2018
Merged

Adding gmusic ability to upload on import #2998

merged 8 commits into from
Aug 10, 2018

Conversation

shuaiscott
Copy link
Contributor

Implemented the import_stages def on gmusic and updated the docs

#2997

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 great! I have a few suggestions inline. Would you also mind writing a quick changelog entry?

self.config.add({
u'auto': False,
})
if self.config['auto'].get(bool):
Copy link
Member

Choose a reason for hiding this comment

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

The .get(bool) bit is not actually necessary here—configuration views get coerced to Booleans automatically.

@@ -62,6 +68,14 @@ def upload(self, lib, opts, args):
self.m.upload(filepaths=files)
ui.print_(u'Your files were successfully added to library')

def autoupload(self, session, task):
items = task.imported_items()
files = [x.path.decode('utf-8') for x in items]
Copy link
Member

Choose a reason for hiding this comment

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

Although I know it’s what the code above does too, this decode call is pretty worrisome and will probably break on some inevitable non-UTF-8 filenames. Can we please factor this out into a helper function so it’s possible to fix both at once?

ui.print_(u'Uploading your files to Google Play Music...')
self.m.upload(filepaths=files)
ui.print_(u'Your files were successfully added to your '
+ 'Google Play Music library')
Copy link
Member

Choose a reason for hiding this comment

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

Using print_ during the import process is likely to interrupt some interactive prompts and such. It’s probably best to use the plugin’s log to emit this output while hiding it by default.

* Fixes based on review and changelog addition
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.

Thank you! One style nit above.

self._log.info(u'Uploading files to Google Play Music...', files)
self.m.upload(filepaths=files)
self._log.info(u'Your files were successfully added to your '
+ 'Google Play Music library')
Copy link
Member

Choose a reason for hiding this comment

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

Travis has a silly whitespace complaint about this line:
https://travis-ci.org/beetbox/beets/jobs/412848100

@sampsyo sampsyo merged commit 2d2428f into beetbox:master Aug 10, 2018
@sampsyo
Copy link
Member

sampsyo commented Aug 10, 2018

Nice work; thank you!

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