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

WIP: Added experimental translation support via gettext. #14

Merged
merged 15 commits into from
Feb 19, 2019

Conversation

anomie31
Copy link

As I don't speak Chinese, someone else will have to translate it.

@DarkLinkXXXX
Copy link

Sorry, I PR'd without testing. I'm distracted by other things right now but I think I can make it work.

@anomie31 anomie31 force-pushed the pr/gettext branch 2 times, most recently from cae23cb to ddc6610 Compare February 11, 2019 00:47
…Chinese, someone else will have to translate it.
@anomie31
Copy link
Author

Okay, I think it works now. Note that before any translations work, you'll have to compile the catalog files with msgfmt locale/$LANG/LC_MESSAGES/music-dl.po -o locale/$LANG/LC_MESSAGES/music-dl.mo. This should be done with setup.py somehow.

@anomie31
Copy link
Author

One of the tests failed and I don't understand why. https://travis-ci.org/0xHJK/music-dl/jobs/491414062#L435

@riptl
Copy link
Contributor

riptl commented Feb 11, 2019

The test fails here.

assert music.avaiable

No idea why

@DarkLinkXXXX
Copy link

One of my peers suggested that it's a geoblocking issue, and I think it's possible. If any of the project maintainers can read this, could you test it in China please?

@0xHJK
Copy link
Owner

0xHJK commented Feb 11, 2019

Sorry. It is my fault. The URL in test_music.py has expired.
Thank you for your contribution🎉. I will modify the test case later.

@anomie31
Copy link
Author

Some friends contributed some translations to English, Japanese, Serbian, and Croatian.

@0xHJK
Copy link
Owner

0xHJK commented Feb 12, 2019

@anomie31 So cool👍

I have modified the test case and fixed the 'url' KeyError.
But I have not used gettext before, it doesn't seem to work.

I tested echo $LANG on Linux and Mac. The output is like en_US.UTF-8
Should I modify the directory name ( Such as en to en_US.UTF-8 ) ?

@riptl
Copy link
Contributor

riptl commented Feb 12, 2019

anomie31#1 I added German translations 🤔

@0xHJK Not all systems are UTF-8 so that's not the right solution imo.
gettext should load en from $LANG=en_US.UTF-8 automatically…

$LANG is commonly overwritten by the $LC_* variables. Can you post a full output of env?

@codecov-io
Copy link

codecov-io commented Feb 12, 2019

Codecov Report

Merging #14 into master will not change coverage.
The diff coverage is 41.17%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master      #14   +/-   ##
=======================================
  Coverage   69.61%   69.61%           
=======================================
  Files          14       14           
  Lines         576      576           
=======================================
  Hits          401      401           
  Misses        175      175
Impacted Files Coverage Δ
music_dl/core.py 61.7% <0%> (ø) ⬆️
music_dl/music.py 87.91% <33.33%> (ø) ⬆️
music_dl/__main__.py 29.41% <50%> (ø) ⬆️

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 a7adb39...46c0962. Read the comment docs.

@anomie31
Copy link
Author

anomie31 commented Feb 12, 2019

@0xHJK Sorry, this is still a work in progress, but it does work. I actually haven't tested it much until now, but it does work after running for each in locale/*/LC_MESSAGES/*.po;do msgfmt "$each" -o "${each%.*}".mo;done. The po files are compiled into a binary search tree format for efficiency. Then you can test it with LANGUAGES=en python3 ./music-dl -k Eminem. It is at this point I realized I missed a lot of strings, because I only found strings within print() statements, but you can still see some parts in English.

…atalog that should have been. Removed non-language text from translation catalog.
@anomie31 anomie31 changed the title Added experimental translation support via gettext. WIP: Added experimental translation support via gettext. Feb 12, 2019
@anomie31
Copy link
Author

@0xHJK I notice on lines like https://github.com/0xHJK/music-dl/blob/master/music_dl/__main__.py#L26 you also have strings in English. Would you like to translate them to Mandarin? I figure it's easier to translate from one language than from multiple.

@0xHJK
Copy link
Owner

0xHJK commented Feb 13, 2019

It does work in some parts.
I seem to find out where the problem is. This may be caused by Chinese punctuation I use. I'll modify them later.

@0xHJK
Copy link
Owner

0xHJK commented Feb 13, 2019

@anomie31 Does it work in this part?

msgstr ""
" -> Source: {idx}{source} #{id}\n"
" -> Title: {title}\n"
" -> Artist: {singer}\n"
" -> Album: {album}\n"
" -> Length: {duration}\n"
" -> Filesize: {size}MB\n"
" -> 比特率: {rating}\n"
" -> URL: {url} \n"

You could test it with ./music-dl -v

@anomie31
Copy link
Author

anomie31 commented Feb 13, 2019

@0xHJK That doesn't work, and it's by design. More text was added after the translation was made, so I just copied it over and marked it as needing work. Apparently the translation isn't used at all when I do so. But it clearly says rating, so I'll just add that.
image

PS: In the future --use-fuzzy can be used in the msgfmt step to translate all messages.

@anomie31
Copy link
Author

Now it's working.
image

@0xHJK
Copy link
Owner

0xHJK commented Feb 14, 2019

@anomie31 I have translated the strings in __main__.py into Chinese and replaced fullwidth colons in music.py. Could you please improve the corresponding translation file? You may need to add initialization statements to __main__.py:main(), just like the music-dl file. Thanks a lot.
And do you think it would be better to save the .mo file directly?

@0xHJK 0xHJK changed the base branch from master to gettext February 15, 2019 03:13
@anomie31
Copy link
Author

And do you think it would be better to save the .mo file directly?
I don't think so. These are binary files that git isn't very good at working with. We could, but it would clobber the git history and increase repository size.

@0xHJK 0xHJK merged commit 94c4248 into 0xHJK:gettext Feb 19, 2019
@anomie31
Copy link
Author

Sorry, I've ghosted this project and the last week, but it looks like you didn't have any more questions. Thank you for merging.

@0xHJK
Copy link
Owner

0xHJK commented Feb 27, 2019

Thank you for your contribution. I have updated the code and completed the translation in Chinese and English. But I have not fully translated them into other languages. Could you please help me? Thanks a lot.
The current branch is https://github.com/0xHJK/music-dl/tree/gettext

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants