Enable connecting to a custom MusicBrainz server

[ghost]

Adresses #172 with a new [musicbrainz] section in the config.

[Freso]

Does this make it default to https://musicbrainz.org/ ? Is it possible to use this with a non-HTTPS localhost?

[Freso]

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 get_musicbrainz_server() and getMusicBrainzSubmitURL().

[Freso]

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.

[RecursiveForest]

I think it might even be nicer to make the "prosaic" comments the msg= param to assertEquals(), as mentioned below.

[Freso]

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?

[ghost]

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.

[Freso]

And again here with the blank lines.

[Freso]

Seems like the flake8 error is in whipper/common/cache.py, so not a file you're touching in the PR, so that can be ignored.

[JoeLametta]

Travis Flake8 build is failing (bare except), but tests do pass.

Fixed that a few minutes ago (master branch).

[MerlijnWajer]

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.

[MerlijnWajer]

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`.

[MerlijnWajer]

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!

[RecursiveForest]

+1 for implementation without using the regular expression, I like the no-scheme suggestion the best.

[ghost]

@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.

[MerlijnWajer]

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.

[RecursiveForest]

I think it would be worth adding a msg= param to assertEquals to say that this a test of the default value.

[RecursiveForest]

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.

[RecursiveForest]

Overall this looks great to me.

[MerlijnWajer]

Thanks!

[JoeLametta]

@naiveaiguy Thank you for the PR!