Enable connecting to a custom MusicBrainz server #210
Conversation
|
Does this make it default to https://musicbrainz.org/ ? Is it possible to use this with a non-HTTPS localhost? |
|
Also, I know that whipper is lacking a lot of tests right now, but it does have some. It would be great if this new code could also have tests. At the very least unit tests for |
whipper/test/test_common_config.py
Outdated
| self._config.write() | ||
|
|
||
| self.assertEquals(self._config.get_musicbrainz_server(), | ||
| '192.168.2.141:5000') |
There was a problem hiding this comment.
I would have these calls listed without empty lines between them, and preferably with a #comment just prior to them to give a more "prosaic" version of what's going on.
There was a problem hiding this comment.
I think it might even be nicer to make the "prosaic" comments the msg= param to assertEquals(), as mentioned below.
whipper/test/test_common_config.py
Outdated
| self.assertEquals(self._config.get_musicbrainz_server(), | ||
| '192.168.2.141:5000') | ||
|
|
||
| self._config._parser.remove_section('musicbrainz') |
There was a problem hiding this comment.
This seems like something that should be done in a "tear down" function/method. What happens if the test breaks before it gets to this call?
There was a problem hiding this comment.
I agree, but I can't see a good way to handle it, since it's not common to the all (or even the majority) of the tests.
whipper/test/test_common_config.py
Outdated
| self._config.write() | ||
|
|
||
| self.assertEquals(self._config.get_musicbrainz_server(), | ||
| '192.168.2.141:5000') |
There was a problem hiding this comment.
And again here with the blank lines.
|
Seems like the flake8 error is in |
Fixed that a few minutes ago (master branch). |
whipper/common/config.py
Outdated
| server = self.get('musicbrainz', 'server') or 'musicbrainz.org' | ||
| server_url = server if re.match(r'^\D*//', server) else '//' + server | ||
| return urlparse(server_url).netloc | ||
|
|
There was a problem hiding this comment.
Maybe this is an alternative without regex?
>>> server
'musicbrainz.org'
>>> server_url = urlparse(server)
>>> if server_url.scheme == '': server_url = urlparse('//' + server)
...
>>> server_url.netloc
'musicbrainz.org'
To be fair, I'd even prefer it to accept only scheme == '' or scheme in ('http', 'https').
This way, afaik, people can set the server to 'ftp://musicbrainz.org' and have it still work.
There was a problem hiding this comment.
Or even only accept no scheme only, since the documentation states:
the MusicBrainz server to connect to, in `host:[port]` format. Defaults to `musicbrainz.org`.
There was a problem hiding this comment.
For the record, you don't have to change it, but we should update the documentation to reflect what we actually accept in any case. Otherwise it looks great!
There was a problem hiding this comment.
+1 for implementation without using the regular expression, I like the no-scheme suggestion the best.
There was a problem hiding this comment.
@MerlijnWajer The alternative would be cleaner, but it doesn't quite work in some cases:
>>> urlparse('192.168.2.97:5000/hello').scheme
192.168.2.97
>>> k = urlparse('192.168.2.97/hello')
>>> k.scheme
''
>>> k.netloc
''set_hostname will only accept host:[port], so perhaps it's best to just add to the docs - The schema and anything after the port number will be stripped out before use. or something similar.
There was a problem hiding this comment.
192.168.2.97/hello is indeed not what the docs specify. If you can only set the hostname I would simply not accept 192.168.2.97/hello as valid URL. If anything, it'll be confusing to people who do set the URI and expect it to have any effect.
whipper/test/test_common_config.py
Outdated
| def test_get_musicbrainz_server(self): | ||
| # Test default return value | ||
| self.assertEquals(self._config.get_musicbrainz_server(), | ||
| 'musicbrainz.org') |
There was a problem hiding this comment.
I think it would be worth adding a msg= param to assertEquals to say that this a test of the default value.
whipper/test/test_common_config.py
Outdated
| '192.168.2.141:5000/hello/world') | ||
| self._config.write() | ||
| self.assertEquals(self._config.get_musicbrainz_server(), | ||
| '192.168.2.141:5000') |
There was a problem hiding this comment.
It might be worth stating that the expected behaviour is for get_musicbrainz_server() to strip the path after the port number in a msg= param, but I don't feel strongly about it.
|
Overall this looks great to me. |
Under the new [musicbrainz] section in the config, you can set a server to connect to. Closes #172.
|
Thanks! |
ghost
deleted the
|
@naiveaiguy Thank you for the PR! |
Adresses #172 with a new
[musicbrainz]section in the config.