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

Added nl language #5354

Merged
merged 11 commits into from
May 7, 2020
Merged

Added nl language #5354

merged 11 commits into from
May 7, 2020

Conversation

berendjan
Copy link
Contributor

No description provided.

Copy link
Member

@kevin-bates kevin-bates left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@berendjan - thank you for your contribution - Dutch localization!

Having not "witnessed" the previous localization additions, it's not entirely clear to me what else is necessary for this. On the surface, everything looks good, but I'd prefer another set of eyes on this particular review.

One reservation I have is that these translations were performed via the Translator Text API. In my experiences with i18n on other projects, this task was typically performed by humans, often with multiple iterations, due to contextual misunderstandings, etc. Assuming you speak/read Dutch, how well do you feel the translations are in preserving the original intent of the given message? It would be good to minimize the number of fixups required because the machine didn't quite interpret intent correctly.

notebook/i18n/README.md Outdated Show resolved Hide resolved
notebook/i18n/README.md Show resolved Hide resolved
@berendjan berendjan changed the title Added nl language [WIP] Added nl language (Spelling checkers needed) Apr 9, 2020
@berendjan
Copy link
Contributor Author

I noticed that the .po template files .pot are out of sync, the lines to which it refers are often incorrect which leads to not translated text. I'm unsure how to generate new .pot files. I presume this is a problem for all translations (French (fr_FR) and Chinees (zh-CN)).

I raised an issue #5356

@berendjan
Copy link
Contributor Author

berendjan commented Apr 9, 2020

Furthermore, we need more volunteer language checkers to iterate the translations.
For this the translations in the .po files in notebook/notebook/i18n/nl/LC_MESSAGES/ need to be checked.
After translating you need to re-generate the .mo files and the .json file.
Navigate your terminal to notebook/notebook/i18n/ and type the following command:

pybabel compile -D notebook -f -l nl -i nl/LC_MESSAGES/notebook.po -o nl/LC_MESSAGES/notebook.mo
pybabel compile -D nbui -f -l nl -i nl/LC_MESSAGES/nbui.po -o nl/LC_MESSAGES/nbui.mo
po2json -p -F -f jed1.x -d nbjs nl/LC_MESSAGES/nbjs.po nl/LC_MESSAGES/nbjs.json

For testing at runtime:
Jupyter Notebook retrieves the language from the LANG variable in the terminal.
You can set is for the NL language by typing export LANG="nl_NL" in the terminal for this translation in case you have your system settings to English.

@berendjan
Copy link
Contributor Author

@berendjan - thank you for your contribution - Dutch localization!

Having not "witnessed" the previous localization additions, it's not entirely clear to me what else is necessary for this. On the surface, everything looks good, but I'd prefer another set of eyes on this particular review.

One reservation I have is that these translations were performed via the Translator Text API. In my experiences with i18n on other projects, this task was typically performed by humans, often with multiple iterations, due to contextual misunderstandings, etc. Assuming you speak/read Dutch, how well do you feel the translations are in preserving the original intent of the given message? It would be good to minimize the number of fixups required because the machine didn't quite interpret intent correctly.

@kevin-bates I checked the translations and changed them where I found necessary, but you are absolutely right and probably iterations are necessary. I changed the commit to a [WIP] and gave some instructions above, I also noticed that the .pot and .po files were out of sync and raised issue #5356.

Copy link
Member

@kevin-bates kevin-bates left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@berendjan - thank you for making an initial "human" pass on the translations. I think that's sufficient to move this forward. I agree with your "ask" for more spell checkers. However, I believe that might not happen for a while and iterating on these messages, post-release, is probably more likely anyway.

I'm good with where this is, but will likely leave this open for a few more days to allow for other reviews.

Thanks again.

Copy link
Contributor

@toonijn toonijn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It should indeed be looked at by a human. A quick glance makes some miss translations clear. Another fundamental question:

  • What is the translation of 'notebook'? 'Notitieblok' feels awkward. 'Werkblad' (worksheet) makes more sense. (Or just keep the English 'notebook')

notebook/i18n/nl/LC_MESSAGES/nbui.po Outdated Show resolved Hide resolved
notebook/i18n/nl/LC_MESSAGES/nbui.po Outdated Show resolved Hide resolved
notebook/i18n/nl/LC_MESSAGES/nbui.po Outdated Show resolved Hide resolved
notebook/i18n/nl/LC_MESSAGES/nbui.po Outdated Show resolved Hide resolved
notebook/i18n/nl/LC_MESSAGES/nbui.po Outdated Show resolved Hide resolved
notebook/i18n/nl/LC_MESSAGES/notebook.po Outdated Show resolved Hide resolved
notebook/i18n/nl/LC_MESSAGES/nbjs.po Outdated Show resolved Hide resolved
notebook/i18n/nl/LC_MESSAGES/nbjs.po Outdated Show resolved Hide resolved
notebook/i18n/nl/LC_MESSAGES/nbjs.po Outdated Show resolved Hide resolved
notebook/i18n/nl/LC_MESSAGES/nbjs.po Outdated Show resolved Hide resolved
@kevin-bates
Copy link
Member

@toonijn - thank you for the great review. I'd like to start down the path of putting together a 6.1.0 release and would love to have this included. @berendjan - would it be possible to respond to Toon's review comments fairly soon?

@kevin-bates kevin-bates mentioned this pull request May 5, 2020
24 tasks
@berendjan
Copy link
Contributor Author

@toonijn Thank you for your input, it's much appreciated. I have to agree that 'notitieblok' which is a notepad in English sounds weird. I changed everything to notebook as I presume people who come in contact with Jupyter Notebook will be more familiar with the English name even when their system settings are Dutch.

If you have time, please look at the changes and comment away! I'll try to address them asap.

Copy link
Contributor

@toonijn toonijn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • "cellenuitvoer" is not a real word. I don't really have een alternative maybe "resultaat van cel", or something like that.
  • What do we call a cell of type 'raw'? We just could keep 'raw'.

notebook/i18n/nl/LC_MESSAGES/nbjs.json Outdated Show resolved Hide resolved
notebook/i18n/nl/LC_MESSAGES/nbjs.json Outdated Show resolved Hide resolved
notebook/i18n/nl/LC_MESSAGES/nbjs.json Outdated Show resolved Hide resolved
notebook/i18n/nl/LC_MESSAGES/nbjs.json Outdated Show resolved Hide resolved
notebook/i18n/nl/LC_MESSAGES/nbjs.json Outdated Show resolved Hide resolved
notebook/i18n/nl/LC_MESSAGES/nbjs.json Outdated Show resolved Hide resolved
notebook/i18n/nl/LC_MESSAGES/nbjs.json Outdated Show resolved Hide resolved
notebook/i18n/nl/LC_MESSAGES/nbjs.json Outdated Show resolved Hide resolved
notebook/i18n/nl/LC_MESSAGES/notebook.po Outdated Show resolved Hide resolved
notebook/i18n/nl/LC_MESSAGES/notebook.po Outdated Show resolved Hide resolved
@toonijn
Copy link
Contributor

toonijn commented May 7, 2020

Looks good to me!

@kevin-bates
Copy link
Member

Fantastic - thank you both @berendjan and @toonijn!

@toonijn - could you please select approve on your review, then I think we'll merge in a day or so to allow others to chime in one last time.

@kevin-bates
Copy link
Member

Thanks @toonijn.

@berendjan - if you're all set, please update the title and we'll take the next step.

@berendjan berendjan changed the title [WIP] Added nl language (Spelling checkers needed) Added nl language (Spelling checkers needed) May 7, 2020
@berendjan
Copy link
Contributor Author

@toonijn @kevin-bates Thanks to you both, I removed [WIP] from the title.

@berendjan berendjan changed the title Added nl language (Spelling checkers needed) Added nl language May 7, 2020
@kevin-bates kevin-bates merged commit e577b6c into jupyter:master May 7, 2020
@blink1073 blink1073 added this to the 6.1 milestone May 15, 2020
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Mar 25, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants