search

abhimanyuPathania / lyrico

A python based command-line lyrics downloader
https://pypi.python.org/pypi/lyrico
Other
15 stars 6 forks source link

Fix lower/upper case extensions on Linux #7

Closed Helgius closed 8 years ago

Helgius commented 8 years ago

On Linux original code can't detect Vorbis files with extensions in lower case. Don't know how it works on Windows. So here is the fix.

abhimanyuPathania commented 8 years ago

can you please attach the Vorbis file that was not being detected?

Helgius commented 8 years ago

None of my ".ogg"-files can be detected by original lyrico code on Linux . All of my files have lower case extension ".ogg". In your code there is "OGG" only, so renaming my files to upper case ".OGG" solves the problem, but it is a hell and misbehavior. I think it's only Linux problem. My code fix searches for upper/lower case extensions more correctly.

abhimanyuPathania commented 8 years ago

After looking into this; yes this is a Linux only issue. Because in Windows the extensions are case-insensitive. Hence the globing detects .OGG and .ogg. My code looks for only .OGG files because the samples I used to test had only uppercase extensions. So I thought that's how that container format worked.

Your code could also have some issues. See _payets comment to a similar solution on SO.

I am thinking of detecting the platform and leave the Windows side as it is. For Linux, simplest solution could be to only detect extensions like .ogg and .OGG and skip rest of the permutations. A file with extension like .oGg can be considered invalid?

Also, adding the .oga format is an entirely separate change. Didn't know it existed. I will look into it and add it into lyrico as well and release an update reflecting all these changes.

Helgius commented 8 years ago

To be honest, this is my first experience in Python :). I've made changes to have working version. I've found your program and I'm really happy with it. It's a great work, cause there is nothing similar for Linux world.

  1. It's up to you to make detection of platform, but why so complicated, if you can add global case insensitivity, may be in better way you specified. Making case insensitive only OGGs is incorrect personally for me :), because I also have a lot of MP3s in lower case.
  2. OGA is the version of extension equal to OGG, opposite to OGV, which is for video format. I don't have such files, but added it for other people that will have them and won't/can't dig into the code to get it working.
abhimanyuPathania commented 8 years ago

Released the updated version. Update to lyrico 0.6.0.

Helgius commented 8 years ago

Thnx