search

emericg / OpenSubtitlesDownload

Automatically find and download the right subtitles for your favorite videos!
https://emeric.io/OpenSubtitlesDownload
GNU General Public License v3.0
607 stars 66 forks source link

fix issue #79 #80

Closed AmitGolden closed 3 years ago

AmitGolden commented 3 years ago

Hi, I really liked this tool! So much that I decided to fix that bug that annoyed me.

Sorry for the different formatting... I have the black formatter running automatically on save.

emericg commented 3 years ago

Hi, thanks for spending time with this issue. #79 seems to be a legitimate problem and your solution seems to be ok, but reformating the whole file makes it extra hard to review the changes, can you resubmit just the code fixing the issue?

As a rule of thumb when working on someone else code, it's easier to just match the style used around your fix. At the end of the day everyone uses a different style, and what's important is being coherent between files (or the top and bottom of a same file ^^) more than the actual style. If you really have to make whitespace or formatting cleanups always make a separate commit.

AmitGolden commented 3 years ago

Sorry for that, the new commit should fix this

emericg commented 3 years ago

Thanks, that's great. I'll try this tomorrow but everything looks good.

AmitGolden commented 3 years ago

Oops, forgot a space :)

AmitGolden commented 3 years ago

Just now I have encountered another error causing CLI mode to fail to download because the subtitles are downloaded and ungzipped manually in CLI mode, instead of using the UNIX tools, so I fixed that

emericg commented 3 years ago

Just now I have encountered another error causing CLI mode to fail to download because the subtitles are downloaded and ungzipped manually in CLI mode, instead of using the UNIX tools, so I fixed that

Yeah but that's not a design issue, that's something that's made on purpose, especially useful for platforms like windows and macOS that don't have these tools preinstalled. Also, doing it in python works better with some network setups like with proxy or VPN.

AmitGolden commented 3 years ago

Oh, I didn't think of that... So how could we solve the problem when sometimes we are provided an invalid encoding? The UNIX way doesn't require an encoding, but the Python way does, so sometimes a subtitle could be downloaded via GTK and not via CLI

emericg commented 3 years ago

So how could we solve the problem when sometimes we are provided an invalid encoding?

What problem are you talking about? Is it still related to #79 ?

The UNIX way doesn't require an encoding, but the Python way does, so sometimes a subtitle could be downloaded via GTK and not via CLI

Invalid encodings suck indeed, and that's still an issue (or a type of issue) we're investigating. We have less encoding problem using wget and unzip because we don't actually touch the files content, but there are still issues, and ultimately the less dependecy used the better.

AmitGolden commented 3 years ago

Yeah you're right it's no longer connected to #79... Reverting those last changes so #79 could be fixed

emericg commented 3 years ago

Great thank you for that PR. I've squashed it and merged it. I have also added a couple of commits on top.

If you have unicode problems, feel free to join the discussion on #78 or on a new issue.