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

Only apply UTF-8 monkey patch when necessary #3573

Merged
merged 6 commits into from
Jan 9, 2018
Merged

Only apply UTF-8 monkey patch when necessary #3573

merged 6 commits into from
Jan 9, 2018

Conversation

stefaang
Copy link
Contributor

@stefaang stefaang commented Jan 5, 2018

  • PR is based on the DEVELOP branch
  • Don't send big changes all at once. Split up big PRs into multiple smaller PRs that are easier to manage and review
  • Read the contribution guide

Fixes #3567

@codecov-io
Copy link

codecov-io commented Jan 5, 2018

Codecov Report

Merging #3573 into develop will decrease coverage by <.01%.
The diff coverage is 21.42%.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop    #3573      +/-   ##
===========================================
- Coverage    33.02%   33.02%   -0.01%     
===========================================
  Files          274      274              
  Lines        34735    34748      +13     
===========================================
+ Hits         11471    11475       +4     
- Misses       23264    23273       +9
Impacted Files Coverage Δ
medusa/init/filesystem.py 33.82% <21.42%> (-2.39%) ⬇️
medusa/tv/series.py 29% <0%> (-0.03%) ⬇️
medusa/subtitles.py 55.94% <0%> (+0.19%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4f6f8e8...ed4df19. Read the comment docs.

@stefaang stefaang changed the title Only apply UTF-8 patch when necessary Only apply UTF-8 monkey patch when necessary Jan 5, 2018
@medariox
Copy link
Contributor

medariox commented Jan 7, 2018

Thanks for the contribution!

We still need to monkey patch to support unicode input for pyOpenSSL, so this solution won't cut it.

Don't you think that there is a problem with the Python implementation in Entware-ng?

@stefaang
Copy link
Contributor Author

stefaang commented Jan 8, 2018

The problem was resolved by skipping the lambda x: x input wrapper, introduced by Medusa. It was failing on closed file object pointers being forwarded as arguments.
I couldn't identify any root cause in the openwrt python build scripts yet.

I'd argue that monkey patching python core modules is a bit more dirty ...
I'll refactor my fix to include the output wrapper (thanks for spotting that by the way)

@medariox
Copy link
Contributor

medariox commented Jan 8, 2018

I didn't mean to say that your solution is dirty in any way, quite the contrary actually. It just needs to handle what I pointed above as well, that's all. We are grateful for any help and contribution.

@stefaang
Copy link
Contributor Author

stefaang commented Jan 8, 2018

Sorry, I couldn't squash the commits due to issues with line endings.
Anyway, glad to help you guys out!

@medariox
Copy link
Contributor

medariox commented Jan 8, 2018

Thanks! The code looks really good. Can you fix the flake8 issues when you get the chance?

@medariox
Copy link
Contributor

medariox commented Jan 8, 2018

Argh, need to remove the whitespaces from the blank lines, see travis issue.

@medariox medariox merged commit be79a12 into pymedusa:develop Jan 9, 2018
@labrys
Copy link
Contributor

labrys commented Jan 10, 2018

Also as an aside, the monkey patching fails for python 3.

@p0psicles p0psicles added this to the 0.1.23 milestone Jan 15, 2018
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.

None yet

5 participants