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

Added subsonic plugin #3001

Merged
merged 16 commits into from Aug 26, 2018

Conversation

Projects
None yet
2 participants
@maffo999
Copy link
Contributor

commented Aug 12, 2018

Added subsonic plugin

@maffo999

This comment has been minimized.

Copy link
Contributor Author

commented Aug 12, 2018

Tox test was passed in my environment

⇒  tox
GLOB sdist-make: /data/lorenzo/git/my_beets/setup.py
py27-test inst-nodeps: /data/lorenzo/git/my_beets/.tox/dist/beets-1.4.8.zip
py27-test installed: beautifulsoup4==4.6.2,beets==1.4.8,certifi==2018.4.16,chardet==3.0.4,click==6.7,cookies==2.2.1,coverage==4.5.1,discogs-client==2.2.1,enum34==1.1.6,Flask==1.0.2,funcsigs==1.0.2,idna==2.7,itsdangerous==0.24,jellyfish==0.6.1,Jinja2==2.10,MarkupSafe==1.0,mock==2.0.0,munkres==1.0.12,musicbrainzngs==0.6,mutagen==1.41.1,nose==1.3.7,nose-show-skipped==0.1,oauthlib==2.1.0,pathlib==1.0.1,pbr==4.2.0,pylast==2.4.0,python-mpd2==1.0.0,pyxdg==0.26,PyYAML==3.13,rarfile==3.0,requests==2.19.1,responses==0.9.0,six==1.11.0,Unidecode==1.0.22,urllib3==1.23,Werkzeug==0.14.1
py27-test runtests: PYTHONHASHSEED='728948394'
py27-test runtests: commands[0] | python -m nose
/data/lorenzo/git/my_beets/.tox/py27-test/lib/python2.7/site-packages/pylast/__init__.py:51: UserWarning: You are using pylast with Python 2. Pylast will soon be Python 3 only. More info: https://github.com/pylast/pylast/issues/265
  UserWarning,
...........................................................................................................................................................................................................................................................................................................................................................................................................................................S.S....................................S...S..............S...S........................................................................................................................................................................SS...........................................................................................................SS............................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................S...SSSSSSSSS............................................................................................................................S....................................S........................................................................................................................................................S..S........................................
----------------------------------------------------------------------
Ran 1808 tests in 44.320s

OK (SKIP=24)
py37-test inst-nodeps: /data/lorenzo/git/my_beets/.tox/dist/beets-1.4.8.zip
py37-test installed: beautifulsoup4==4.6.2,beets==1.4.8,certifi==2018.4.16,chardet==3.0.4,click==6.7,cookies==2.2.1,coverage==4.5.1,discogs-client==2.2.1,Flask==1.0.2,idna==2.7,itsdangerous==0.24,jellyfish==0.6.1,Jinja2==2.10,MarkupSafe==1.0,mock==2.0.0,munkres==1.0.12,musicbrainzngs==0.6,mutagen==1.41.1,nose==1.3.7,nose-show-skipped==0.1,oauthlib==2.1.0,pbr==4.2.0,pylast==2.4.0,python-mpd2==1.0.0,pyxdg==0.26,PyYAML==3.13,rarfile==3.0,requests==2.19.1,responses==0.9.0,six==1.11.0,Unidecode==1.0.22,urllib3==1.23,Werkzeug==0.14.1
py37-test runtests: PYTHONHASHSEED='728948394'
py27-flake8 inst-nodeps: /data/lorenzo/git/my_beets/.tox/dist/beets-1.4.8.zip
py27-flake8 installed: beets==1.4.8,configparser==3.5.0,enum34==1.1.6,flake8==3.5.0,flake8-blind-except==0.1.1,flake8-coding==1.3.0,flake8-future-import==0.4.5,flake8-polyfill==1.0.2,jellyfish==0.6.1,mccabe==0.6.1,munkres==1.0.12,musicbrainzngs==0.6,mutagen==1.41.1,pathlib==1.0.1,pep8-naming==0.7.0,pycodestyle==2.3.1,pyflakes==1.6.0,PyYAML==3.13,six==1.11.0,Unidecode==1.0.22
py27-flake8 runtests: PYTHONHASHSEED='728948394'
py27-flake8 runtests: commands[0] | flake8 --min-version 2.7 beets beetsplug beet test setup.py docs
docs inst-nodeps: /data/lorenzo/git/my_beets/.tox/dist/beets-1.4.8.zip
docs installed: alabaster==0.7.11,Babel==2.6.0,beets==1.4.8,certifi==2018.4.16,chardet==3.0.4,docutils==0.14,enum34==1.1.6,idna==2.7,imagesize==1.0.0,jellyfish==0.6.1,Jinja2==2.10,MarkupSafe==1.0,munkres==1.0.12,musicbrainzngs==0.6,mutagen==1.41.1,packaging==17.1,Pygments==2.2.0,pyparsing==2.2.0,pytz==2018.5,PyYAML==3.13,requests==2.19.1,six==1.11.0,snowballstemmer==1.2.1,Sphinx==1.7.6,sphinxcontrib-websupport==1.1.0,typing==3.6.4,Unidecode==1.0.22,urllib3==1.23
docs runtests: PYTHONHASHSEED='728948394'
docs runtests: commands[0] | sphinx-build -W -q -b html docs /data/lorenzo/git/my_beets/.tox/docs/tmp/html
____________________________________________________________________________________________________ summary ____________________________________________________________________________________________________
  py27-test: commands succeeded
  py37-test: commands succeeded
  py27-flake8: commands succeeded
  docs: commands succeeded
  congratulations :)
`
@sampsyo
Copy link
Member

left a comment

Looking good! I have some suggestions that I've left inline.

Also, we have a number of similar plugins around that update other various music library services, and they're generally called *update. See mpdupdate, embyupdate, plexupdate, sonosupdate, and kodiupdate. So, even though it's kind of long, I think we should probably change the name of this one to subsonicupdate.

@@ -18,6 +18,7 @@ New features:
:user:`jams2`
* Automatically upload to Google Play Music library on track import.
:user:`shuaiscott`
* Added Subsonic automatic library update plugin

This comment has been minimized.

Copy link
@sampsyo

sampsyo Aug 13, 2018

Member

This can be nicely linked if you instead use:

Add a new :doc:`/plugins/subsonic` that can automatically update your Subsonic library.
print("Operation completed successfully!")
else:
self._log.error(u'Unknown error code returned from server.')
print("Unknown error code returned from server.")

This comment has been minimized.

Copy link
@sampsyo

sampsyo Aug 13, 2018

Member

I don't think there's any need to both log and print these messages—just using self._log.error should suffice.

This comment has been minimized.

Copy link
@maffo999

maffo999 Aug 13, 2018

Author Contributor

Sorry, this is left behind from my "debugging". I wanted to remove the "print" statements but forgot.

@@ -0,0 +1,69 @@
# -*- coding: utf-8 -*-

This comment has been minimized.

Copy link
@sampsyo

sampsyo Aug 13, 2018

Member

Please include our standard license block comment at the top of the file. Then the documentation you've written below can go into a docstring instead of comments. (See some other plugins for examples.)

host = self.config['host'].get()
port = self.config['port'].get()
user = self.config['user'].get()
passw = self.config['pass'].get()

This comment has been minimized.

Copy link
@sampsyo

sampsyo Aug 13, 2018

Member

Please use as_str() instead of get() to assert that values in the config are actually strings. (Unless you want the port to be a number, in which case use as_number.)

user = self.config['user'].get()
passw = self.config['pass'].get()
r = string.ascii_letters + string.digits
url = "http://"+str(host)+":"+str(port)+"/rest/startScan"

This comment has been minimized.

Copy link
@sampsyo

sampsyo Aug 13, 2018

Member

There's no need to use str() here. Also, consider using format strings:

url = "http://{}:{}/rest/startScan".format(host, port)
salt = "".join([random.choice(r) for n in range(6)])
t = passw + salt
token = hashlib.md5()
token.update(t.encode('utf-8'))

This comment has been minimized.

Copy link
@sampsyo

sampsyo Aug 13, 2018

Member

Consider breaking up these lines of code and adding some comments to say what you're doing: generating a salt, with which you hash a password to send to the server.

Subsonic Plugin
================

``Subsonic`` is a very simple plugin for beets that lets you automatically

This comment has been minimized.

Copy link
@sampsyo

sampsyo Aug 13, 2018

Member

Use lower-case subsonic when you refer to the name of the plugin that the user should put into their configuration file.


def loaded(self):
host = self.config['host'].get()
port = self.config['port'].get()

This comment has been minimized.

Copy link
@sampsyo

sampsyo Aug 13, 2018

Member

Consider using self.config.add() to provide defaults for these options. For example, host might default to 127.0.0.1, and it looks like there's a reasonable default port too. See other plugins for examples of how to provide defaults.

This comment has been minimized.

Copy link
@maffo999

maffo999 Aug 13, 2018

Author Contributor

I avoided that as people could have (like me) the two software running on different machines.
While I agree that a default configuration is better than none at all, it still poses the risk of having a non-working plugin.

This comment has been minimized.

Copy link
@sampsyo

sampsyo Aug 13, 2018

Member

Hmm... I’m not quite sure I follow. Why would having multiple instances be incompatible with having a default? I imagine most people will want to customize most things, but perhaps not all the options all of the time.

@maffo999

This comment has been minimized.

Copy link
Contributor Author

commented Aug 13, 2018

Thanks for the feedback @sampsyo .
I called the plugin simply "Subsonic" not really because of the mouthful which "subsonicupdate" is but rather because in the future I might look into integrating other features.
I am also happy to rename it for now and then create a new plugin later on with the additional features I would like to implement, let me know which you think it's best.

@sampsyo

This comment has been minimized.

Copy link
Member

commented Aug 13, 2018

Yeah, let’s do it. Keeping the plugin focused, at least for now, would be nice. (As you can see, we have multiple plugins that interact with MPD too.)

@maffo999

This comment has been minimized.

Copy link
Contributor Author

commented Aug 25, 2018

Hi @sampsyo
did you check the changes I applied and can you confirm if anything else is needed before you can merge it?

@sampsyo
Copy link
Member

left a comment

Looking good! Here are some comments on the current version.

def __init__(self):
super(SubsonicUpdate, self).__init__()

# Set default configuration values

This comment has been minimized.

Copy link
@sampsyo

sampsyo Aug 25, 2018

Member

Comments should be indented with the code they apply to.

This comment has been minimized.

Copy link
@maffo999

maffo999 Aug 26, 2018

Author Contributor

Corrected.

response = requests.post(url, params=payload)

# Log an eventual error reported by the server or success on status code 200
if (response.status_code == 0):

This comment has been minimized.

Copy link
@sampsyo

sampsyo Aug 25, 2018

Member

No parentheses are needed around if conditions in Python.

This comment has been minimized.

Copy link
@maffo999

maffo999 Aug 26, 2018

Author Contributor

Corrected.

elif (response.status_code == 200):
self._log.info('Operation completed successfully!')
else:
self._log.error(u'Unknown error code returned from server.')

This comment has been minimized.

Copy link
@sampsyo

sampsyo Aug 25, 2018

Member

You might consider using a constant mapping from codes to messages. For example, you could define (at the top of this file):

ERROR_MESSAGES = {
    0: u'Generic error, please try again later.'
}

and use that to look up the right message here.

This comment has been minimized.

Copy link
@maffo999

maffo999 Aug 26, 2018

Author Contributor

I see what you mean, I will take this in consideration for the next "version" of the plugin.

- **host**: localhost
- **port**: 4040
- **user**: admin
- **pass**: admin

This comment has been minimized.

Copy link
@sampsyo

sampsyo Aug 25, 2018

Member

The usual style in the beets documentation is to put the defaults inline. For example, you can say:

The hostname (or IP address) for the Subsonic server. Default: localhost.

This comment has been minimized.

Copy link
@maffo999

maffo999 Aug 26, 2018

Author Contributor

Corrected.

super(SubsonicUpdate, self).__init__()

# Set default configuration values
self.config['subsonic'].add({

This comment has been minimized.

Copy link
@sampsyo

sampsyo Aug 25, 2018

Member

I'm not sure you mean self.config['subsonic'] here, which will expect something like this in the configuration YAML:

subsonicupdate:
    subsonic:
        host: ...
        port: ...

You can either use self.config alone (to put the options directly under the plugin name) or use the global configuration, as in config['subsonic'] after importing the global config object, to create a top-level subsonic entry.

This comment has been minimized.

Copy link
@maffo999

maffo999 Aug 26, 2018

Author Contributor

This was due to my misunderstanding of how the config works, removed the "self" references and imported "config" from beets.

salt = "".join([random.choice(r) for n in range(6)])
t = passw + salt

# Hash the password making sure it's UTF-8 format

This comment has been minimized.

Copy link
@sampsyo

sampsyo Aug 25, 2018

Member

I'm not sure "making sure it's UTF-8 format" makes sense as a comment here. What's actually happening is that the code is hashing the UTF-8 bytes for the password string. Maybe just leave this detail out?

This comment has been minimized.

Copy link
@maffo999

maffo999 Aug 26, 2018

Author Contributor

Removed comment.

}
url = "http://{}:{}/rest/startScan".format(host, port)

# Send the request and store the response

This comment has been minimized.

Copy link
@sampsyo

sampsyo Aug 25, 2018

Member

This is a case where I think the comment might be a little too much detail—this line is clear enough by itself.

This comment has been minimized.

Copy link
@maffo999

maffo999 Aug 26, 2018

Author Contributor

Removed comment.

Io Ii
@maffo999

This comment has been minimized.

Copy link
Contributor Author

commented Aug 26, 2018

Addressed the last issues/suggestions.

Io Ii

maffo999 and others added some commits Aug 26, 2018

@sampsyo

This comment has been minimized.

Copy link
Member

commented Aug 26, 2018

I resolved a merge conflict and refined the changelog entry text to match our usual style.

It looks like you're experimenting with logging stuff, right? When that's sorted out, I think we're ready to merge.

@maffo999

This comment has been minimized.

Copy link
Contributor Author

commented Aug 26, 2018

Well to be honest this is a whole experiment to me 😄
I will look better into logging and work on that for the next version.

@sampsyo

This comment has been minimized.

Copy link
Member

commented Aug 26, 2018

OK, cool. Please either leave the logging statements in there (uncommented) or remove them for now—either way, we can revisit improvements in the future.

@sampsyo

This comment has been minimized.

Copy link
Member

commented Aug 26, 2018

That’ll do it. Thanks again!!

@sampsyo sampsyo merged commit a6c4f0a into beetbox:master Aug 26, 2018

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.