Allow soundtouch to play https content too #16713
Conversation
homeassistant
added
platform: media_player.soundtouch
small-pr
|
Home assistant website is broken, so cannot sign CLA... |
| @@ -297,7 +297,7 @@ def media_album_name(self): | |||
| def play_media(self, media_type, media_id, **kwargs): | |||
| """Play a piece of media.""" | |||
| _LOGGER.debug("Starting media with media_id: %s", media_id) | |||
| if re.match(r'http://', str(media_id)): | |||
| if re.match(r'https?://', str(media_id)): | |||
There was a problem hiding this comment.
You should compile this statement on top.
URL = re.compile(r"..")
...
if URL.match(media_id):
There was a problem hiding this comment.
compiling regexes is generally a best practice, but as this is a function which will be called rarely it seems like unnecessary overhead here to compile a regex for just one execution no?
There was a problem hiding this comment.
Yes. You call this regex every time where some one play a URL with this platform. Maybe you use it 1-2 in a month but other could use it 3-6 per day
There was a problem hiding this comment.
@pvizeli If we create a URL object here, it will exist for that one match, and as soon as the play_media function is completed, it will go out of scope, and be scrapped... the regular expression is used exactly one time per call of the function, and so there is no advantage in compiling it. Compiling regular expressions is only an advantage when they are being used many times within a context.
There was a problem hiding this comment.
I think it's fine to just do it here.
|
Hi all, Are we sure this is going to work? It seems from @CharlesBlonde's libsoundtouch that https can't be played. |
|
Aw shoot... I think you might be right... https://github.com/CharlesBlonde/libsoundtouch#070---20170705 |
|
Thanks. @balloob is it possible to unmerge? This will break things with Bose Soundtouch. |
|
You could have spend the time to make a PR in the time it takes to write that comment 😉 |
|
(it's also not breaking anything) |
haha. I'm not that expert yet :) |
ghost
added
integration: soundtouch
and removed
platform: media_player.soundtouch
labels
No description provided.