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

Support BSPlayer Subtitles provider #996

Merged
merged 15 commits into from
Sep 16, 2024
Merged

Support BSPlayer Subtitles provider #996

merged 15 commits into from
Sep 16, 2024

Conversation

Nyaran
Copy link
Contributor

@Nyaran Nyaran commented Apr 5, 2020

No description provided.

@ratoaq2
Copy link
Collaborator

ratoaq2 commented May 3, 2020

Thank you for the contribution. I'll review this whenever I can

@kepi
Copy link

kepi commented Dec 21, 2020

Is there any way how to help with merging this? The code seams clean enough, but I'm not familiar with the code enough.

@luzpaz
Copy link

luzpaz commented Apr 22, 2023

bump

@Nyaran
Copy link
Contributor Author

Nyaran commented Jul 28, 2024

As subliminal seems alive again, I'm working again on this to sync with the latest changes

@Nyaran Nyaran marked this pull request as draft July 28, 2024 19:03
@Nyaran Nyaran marked this pull request as ready for review July 28, 2024 22:06
@Nyaran
Copy link
Contributor Author

Nyaran commented Jul 28, 2024

I think is ready to review 😃

@getzze
Copy link
Collaborator

getzze commented Jul 28, 2024

Thanks!
You can find some guidance about the tools we use for linting and typing (ruff/pre-commit and mypy) in the CONTRIBUTING file

The errors with the tests for python < 3.12 seem to be a missing line at the top of the files: from __future__ import annotations

@Nyaran
Copy link
Contributor Author

Nyaran commented Jul 30, 2024

It's seems that there is something wrong with Python 3.9 and below, the response from BSPlayer is recorded as "binary", but from Python 3.10, is correctly recorded as a string (check tests/cassettes/bsplayer/test_login.yaml and tests/cassettes/bsplayer.py3.9/test_login.yaml)

The solution I found is to have two different recordings, one for Python <=3.9 (named bsplayer.py3.9) and another for python >=3.10 (named bsplayer)

Do you have any thoughts about this?

@getzze
Copy link
Collaborator

getzze commented Jul 31, 2024

You can try adding the decode_compressed_response=True argument when initializing VCR in test_bsplayer.py, see the other providers.

@Nyaran
Copy link
Contributor Author

Nyaran commented Jul 31, 2024

You can try adding the decode_compressed_response=True argument when initializing VCR in test_bsplayer.py, see the other providers.

That is! Thank you! I didn't see that.

I also fixed the mypy warnigns, so now is really ready for review

Pd. The mypy analysis is not documented on the contribution guide

Copy link

github-actions bot commented Aug 1, 2024

Coverage report

Click to see where and how coverage changed

FileStatementsMissingCoverageCoverage
(new stmts)
Lines missing
  subliminal/providers
  bsplayer.py 107-111, 116-120, 125-129, 171-175, 177, 193, 218
  subliminal/refiners
  hash.py
Project Total  

This report was generated by python-coverage-comment-action

@Nyaran
Copy link
Contributor Author

Nyaran commented Aug 15, 2024

Hey! Anything else I need to do before this PR can be reviewed/merged?

@getzze
Copy link
Collaborator

getzze commented Sep 7, 2024

Hi @Nyaran,
Thanks a lot for your contribution!

I made some modifications about types and making sure connection errors are correctly handled.

I have one question though, you initialize a self.server attribute to a ServerProxy but it is never used, was it a remnant of older code?

@getzze
Copy link
Collaborator

getzze commented Sep 8, 2024

(failing CI for docs does not seem to be related to this PR)

@Nyaran
Copy link
Contributor Author

Nyaran commented Sep 8, 2024

I have one question though, you initialize a self.server attribute to a ServerProxy but it is never used, was it a remnant of older code?

Looks like remnant code, should be safe to remove it

@getzze
Copy link
Collaborator

getzze commented Sep 8, 2024

Ok, LGTM!
@ptrcnull do you have time to have a quick look?

@getzze getzze self-assigned this Sep 10, 2024
@getzze getzze merged commit 5f7a3e8 into Diaoul:main Sep 16, 2024
18 of 20 checks passed
@getzze
Copy link
Collaborator

getzze commented Sep 16, 2024

Merged!

Thanks again @Nyaran for the PR and updating to the latest change!!

@luzpaz
Copy link

luzpaz commented Sep 16, 2024

Congrats! Long time coming

@Nyaran Nyaran deleted the bsplayer branch September 16, 2024 23:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants