Fix tvplayer plugin and tests #2802
Conversation
Fix site login token in authenticate() and login_token_re Fix attributes parsing in _get_stream_attrs() Fix return of correct stream URL in _get_stream_data() Fix DRM checking in _get_streams()
Fix mock_get_stream_data.return_value Fix page_resp.text Fix mock_get_stream_data.assert_called_with
Codecov Report
@@ Coverage Diff @@
## master #2802 +/- ##
==========================================
- Coverage 52.80% 52.79% -0.01%
==========================================
Files 248 248
Lines 15461 15458 -3
==========================================
- Hits 8164 8161 -3
Misses 7297 7297 |
There was a problem hiding this comment.
I'm geoblocked and can't verify your plugin changes, but there are a couple of things I'd like to see changed before this pull request gets merged, for the sake of PEP8 compatibility and code legibility. Some of it is also from previous changes of the plugin, so please be so kind and fix these things. Thank you.
Since Github doesn't allow me to add review comments to lines which are not included in the PR's own diff, I'll have to post an additional diff in this comment here.
diff --git a/src/streamlink/plugins/tvplayer.py b/src/streamlink/plugins/tvplayer.py
index 664911e3..acebd201 100644
--- a/src/streamlink/plugins/tvplayer.py
+++ b/src/streamlink/plugins/tvplayer.py
@@ -96,9 +107,11 @@ class TVPlayer(Plugin):
if "enter your postcode" in res.text:
self.logger.info("Setting your postcode to: {0}. "
"This can be changed in the settings on tvplayer.com", self.dummy_postcode)
- res = self.session.http.post(self.update_url,
- data=dict(postcode=self.dummy_postcode),
- params=dict(return_url=self.url))
+ res = self.session.http.post(
+ self.update_url,
+ data=dict(postcode=self.dummy_postcode),
+ params=dict(return_url=self.url)
+ )
stream_attrs = self._get_stream_attrs(res)
if stream_attrs:Also, the plugin is still using the deprecated self.logger calls. Those should be turned into native log calls, with the following diff added.
diff --git a/src/streamlink/plugins/tvplayer.py b/src/streamlink/plugins/tvplayer.py
index 664911e3..f344e665 100644
--- a/src/streamlink/plugins/tvplayer.py
+++ b/src/streamlink/plugins/tvplayer.py
@@ -1,4 +1,5 @@
#!/usr/bin/env python
+import logging
import re
from streamlink.plugin import Plugin, PluginArguments, PluginArgument
@@ -7,6 +8,9 @@ from streamlink.plugin.api import useragents
from streamlink.stream import HLSStream
+log = logging.getLogger(__name__)
+
+
class TVPlayer(Plugin):
api_url = "https://v1-streams-elb.tvplayer-cdn.com/api/live/stream/"
stream_url = "https://live.tvplayer.com/stream-live.php"
|
Thanks for the code fixes. This is much better now. |
|
@bastimeyer Anything else? If not let's get this merged. |
|
If it's working according to #2456, then go ahead and merge. I can't say much here due to being geo-blocked. The rest is looking fine I guess. |
|
Are you going to merge this into master? I completely understand that you may wish to make improvements to the code, but surely that can be done in another PR? In the meantime, master has no working tvplayer plugin... |
|
@mkbloke We've got a huge PR for flake8 fixes here: #2804 that I just merged (had to get this in before anything else because it had so many changes). Sorry about the delay on getting this in. I was hesitant on the order because this now has some conflicts because of the flake8 support and if I had merged this first the flake8 support would have been messed up. Can you fix those up real quick and then we'll get this merged? Sorry about the inconvenience. |
|
Thanks for the response, @gravyboat. I had somewhat realised that the flake8 stuff may have been an issue. As far as I can see (and having checked locally), the PR does not invalidate flake8 compliance for the plugin. I did not get an answer on flake8 regarding: #2802 (comment) for test_tvplayer.py, so that's still an unknown. |
Just make it so that it doesn't produce any linting errors/warnings and that the test is legible. Everything else is up to you. The merge conflicts are here because your PR now has a diff from an older commit, even though the resulting changes are probably the same. It should be relatively simple to resolve though. If you need help with solving the merge conflicts and rebasing your PR branch, just ask. |
|
@bastimeyer, thanks for the response. Can you advise how to resolve Thanks. |
…o tvplayer-site-change-2020-02
|
Shall I just delete that single long line? It would solve the E501 and W605 issues flagged by flake8. The content in that line is not required for the test to pass. |
|
Looks good, thanks @mkbloke and @bastimeyer! |
* Update for major tvplayer.com site change
* Update for major tvplayer.com site change
* Update for major tvplayer.com site change
Let's try again.
closes #2456