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.

Sign up for GitHub

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

Jump to bottom

Preliminary i18n implementation as outlined in Jep16 #2140

Merged
merged 27 commits into from
Jul 11, 2017
Merged

Preliminary i18n implementation as outlined in Jep16 #2140

merged 27 commits into from
Jul 11, 2017

Conversation

JCEmmons
Copy link
Contributor

@JCEmmons JCEmmons commented Feb 6, 2017

I wanted to get a preliminary PR out here for you all to take a look at so you can see the current progress and also so I can get some help on some of the issues. Please read the i18n README on this branch for instructions on how to get started.

This is still a work in progress, and I am not a professional translator, so I don't claim everything to be perfect just yet. Take a look and let me know what you think.

@takluyver
Copy link
Member

Thanks for working on this!

I'm aiming to release 5.0 soon (hopefully this week), so I think this will probably miss that release, but I'd like to try to get it in for 5.1.

@JCEmmons
Copy link
Contributor Author

JCEmmons commented Feb 6, 2017 via email

@takluyver
Copy link
Member

We don't really have a schedule for releases, I'm afraid. If the changes in 5.0 cause some problems we need to fix, there might be a 5.1 in a couple of weeks, but in that case most of the new features planned for 5.1 would be targeted to 5.2 instead.

If you want some target dates, I'd hope to have the JEP and this infrastructure merged by the end of the month.

@minrk minrk added this to the 5.1 milestone Feb 7, 2017
@JCEmmons
Copy link
Contributor Author

How do we go about making this so it can be merged? I've still got a few small things that can be worked on here, but the vast majority of the work here is complete. Please advise.

@JCEmmons
Copy link
Contributor Author

JCEmmons commented Apr 3, 2017

Still waiting.... @takluyver can you comment pls?

@takluyver
Copy link
Member

  • We're still getting notebook 5.0 out, so this has been waiting for that to happen.
  • There are merge conflicts now - can you rebase the branch on master? If you're not familiar with rebasing, I can provide a brief outline of how I usually do it.

@JCEmmons
Copy link
Contributor Author

JCEmmons commented Apr 3, 2017 via email

@takluyver
Copy link
Member

To rebase and resolve merge conflicts, I usually:

  • Ensure master is up to date - git checkout master and git pull (from jupyter/notebook, rather than your fork on Github)
  • Check out this branch again - git checkout jep16
  • Run git rebase master
  • Each time it reaches a conflict, it will stop and tell you about it. Run git status to see which files have conflicts. Fix them in a text editor (look for markers like >>>>> and <<<<<). Then git add the files you've fixed, and run git rebase --continue.
  • Once it has got through all the commits, it will stop without a message about conflicts.
  • Then you need to force push the rebased branch - git push -f <my_remote> jep16 (replace <my_remote> with the name it has for your Github fork of the repo.

@JCEmmons
Copy link
Contributor Author

I'll work on the travis-ci failures next week.

takluyver
takluyver reviewed Apr 19, 2017
View reviewed changes
notebook/notebookapp.py Outdated
@@ -969,7 +972,7 @@ def _notebook_dir_validate(self, proposal):
# If we receive a non-absolute path, make it absolute.
value = os.path.abspath(value)
if not os.path.isdir(value):
raise TraitError("No such notebook dir: %r" % value)
raise TraitError(_("No such notebook dir: %r") % value)
Copy link
Member

Choose a reason for hiding this comment

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

Unfortunately here the convention of using _ for translatable strings has clashed with the Python convention of using _ for a variable that won't be used.

@JCEmmons
Copy link
Contributor Author

I'm looking at the logs from the js tests in Travis CI, and I have no idea how to go about debugging here. Can someone shed some light on what might be going on?

@takluyver
Copy link
Member

😕 the errors look like this - anyone got any ideas?

Test file: /home/travis/build/jupyter/notebook/notebook/tests/services/session.js
Timeout for http://localhost:8888/a@b/
Is the notebook server running?
FAIL "#kernel-python2 a, #kernel-python3 a" still did not exist in 10000ms
#    type: uncaughtError
#    file: /home/travis/build/jupyter/notebook/notebook/tests/services/session.js
#    error: "#kernel-python2 a, #kernel-python3 a" still did not exist in 10000ms

#    stack: not provided

@takluyver
Copy link
Member

Aha, I've found the issue. You've got utils.i18n in a few places where it should be just i18n.

@JCEmmons
Copy link
Contributor Author

Thanks @takluyver for your help! That's what happens when you try to refactor things.....

@takluyver
Copy link
Member

The tests are now producing a whole lot of ReferenceError: Can't find variable: utils [object Arguments]

@JCEmmons
Copy link
Contributor Author

JCEmmons commented Apr 25, 2017 via email

@takluyver
Copy link
Member

Even without running the tests, you may be able to trigger some of these errors by running the notebook in a browser with the Javascript console open.

@JCEmmons
Copy link
Contributor Author

It would be really nice if we could go ahead and get this merged, as I'm getting tired of resolving merge conflicts.

@takluyver
Copy link
Member

Sorry for the delay @JCEmmons . It's the end of the day over here now, but I'll try to have a look at this tomorrow.

@takluyver
Copy link
Member

Sorry, most of today got taken up working on Windows packaging of some magnetic simulation code. I haven't forgotten about this, though.

takluyver
takluyver reviewed May 17, 2017
View reviewed changes
notebook/i18n/de/LC_MESSAGES/notebook.po Outdated
"X-Generator: Poedit 1.8.11\n"

msgid "The Jupyter Notebook requires tornado >= 4.0"
msgstr "Der Jupyter Notizblock erfordert Tornado >= 4.0"
Copy link
Member

Choose a reason for hiding this comment

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

I think I'd treat 'Notebook' as part of a name and not translate it. Likewise 'Widgets' a few lines below.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Moot since we're removing the machine translated German.

takluyver
takluyver reviewed May 17, 2017
View reviewed changes
notebook/i18n/de/LC_MESSAGES/notebook.po Outdated
" This launches a Tornado based HTML Notebook Server that serves up an "
"HTML5/Javascript Notebook client."
msgstr ""
"Die Jupyter HTML Notebook.n n dies startet einen tornadobasierten HTML "
Copy link
Member

Choose a reason for hiding this comment

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

The newlines have been converted to n.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Moot since we're removing the machine translated German. This was an artifact of our old MT engine.

takluyver
takluyver reviewed May 17, 2017
View reviewed changes
notebook/i18n/de/LC_MESSAGES/notebook.po Outdated

msgid "Set the Access-Control-Allow-Credentials: true header"
msgstr ""
"Setzen Sie die Zugangskontrolle erlauben Sie Ausweispapiere: Wahre Überschrift"
Copy link
Member

Choose a reason for hiding this comment

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

Access-Control-Allow-Credentials shouldn't be translated again - the HTTP protocol uses English words whatever the locale of the user is.

I know the translations aren't the focus here, but if we're going to merge it with German translations in, I think it's worth doing a bit of basic QA.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Moot since we're removing the machine translated German.

takluyver
takluyver reviewed May 17, 2017
View reviewed changes
notebook/i18n/de/LC_MESSAGES/notebook.po Outdated
msgstr "Ob, dem Benutzer zu erlauben, den Notizblock als Wurzel zu führen."

msgid "The default URL to redirect to from `/`"
msgstr "Die versäumte URL, zu der man umleiten kann, von"
Copy link
Member

Choose a reason for hiding this comment

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

The / has gone missing.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Moot since we're removing the machine translated German.

takluyver
takluyver reviewed May 17, 2017
View reviewed changes
notebook/i18n/de/LC_MESSAGES/notebook.po Outdated
msgstr "Die Akte, wo das Keksgeheimnis gespeichert wird."

msgid "Writing notebook server cookie secret to %s"
msgstr "Notizblockserverkeksgeheimnis auf %s schreiben"
Copy link
Member

Choose a reason for hiding this comment

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

My German coworker is laughing uproariously at this translation. I think that while 'keks' is the translation of an edible cookie, an HTTP cookie is called a cookie.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Machine translation can be amusing at times...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Moot since we're removing the machine translated German.

takluyver
takluyver reviewed May 17, 2017
View reviewed changes
notebook/static/base/js/dialog.js Outdated
@@ -86,7 +86,7 @@ define(function(require) {
var button = $("<button/>")
.addClass("btn btn-default btn-sm")
.attr("data-dismiss", "modal")
.text(label);
.text(i18n.translate(label).fetch());
Copy link
Member

Choose a reason for hiding this comment

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

Should these labels be translated where they're defined, rather than here?

Copy link
Member

Choose a reason for hiding this comment

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

I see why this is in place now. I'd still like to find a way to avoid duplicating the button labels in the files where they are defined, but that needn't be solved in this PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, it's kind of confusing until you understand the dialog.js code, but it does work properly, as long as you have the button labels defined properly.

takluyver
takluyver reviewed May 17, 2017
View reviewed changes
notebook/static/base/js/i18n.js Outdated
'jed',
'moment',
'json!../../../i18n/nbjs.json',
'json!../../../i18n/de/LC_MESSAGES/nbjs.json',
Copy link
Member

Choose a reason for hiding this comment

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

Is there a way to only load the data for languages that are needed, rather than pre-loading everything before selecting one?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, there is, but it usually requires writing a JS plugin in order to do it. I've run into a similar problem in other projects I have worked on. What you really want here is a synchronous load, which is kind of a no-no in JS these days. It can be done, and I'm willing to contribute it if/when people actually need a real translation done.

Copy link
Member

Choose a reason for hiding this comment

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

Maybe it's possible to use require in a way that it doesn't fail if it can't find a module? It looks like we can get the language code from navigator before we call define(), so we could stick it into a URL - but we don't know at that point if we're asking for a supported language.

I think I'd like to get this worked out before we merge the translation machinery - sorry for another delay, but loading the messages seems pretty fundamental, and the current mechanism is one we know we'll need to replace.

The json! thing is pretty neat, though - I didn't know about that before.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK - it required a bit of reworking, but I finally got this to work. What you will see is a new plugin called i18nload that loads the appropriate JSON data, and then a promise called msg_promise that is returned. Once msg_promise is resolved, then you can retrieve i18n messages as normal. If a supported translation is not requested or not available, then the promise will resolve with just the default data, and everything works the same as before....

takluyver
takluyver reviewed May 17, 2017
View reviewed changes
notebook/static/base/js/namespace.js Outdated

// deprecated since 4.0, remove in 5+
var IPython = Jupyter;
// Copyright (c) Jupyter Development Team.
Copy link
Member

Choose a reason for hiding this comment

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

Why is the diff showing that this whole file has been changed, when it looks like most of it hasn't? This would mess up tools like git blame.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure... Looks like just formatting diffs between spaces/tabs. I will replace with the old version in my next commit.

@minrk
Copy link
Member

minrk commented Jul 11, 2017

@JCEmmons thanks for addressing the review! I think this is in good shape to merge, with one exception I just noticed while testing this out locally: Some of the changes here have mixed in some tab indentation instead of spaces, and added Windows CRLF line endings in a few places. This is part of why the diff on a few files looks so large. You can tell git to handle CRLF automatically with:

git config --global core.autocrlf true

to avoid this sort of problem in the future. If you check the "Allow edits from maintainers" box on the right:

screen shot 2017-07-11 at 10 43 25

I can do the last conflict resolution and fix these little things and get this merged. Or you can pull from my branch, if you prefer.

@JCEmmons
Copy link
Contributor Author

Hi @minrk and thanks.... Allow edits from maintainers is already enabled, so if you can handle the last merge conflict and do the merge I would appreciate it greatly!!! Thanks......

@minrk
Copy link
Member

minrk commented Jul 11, 2017

We can continue to iterate on this in master as things come up. Thanks for your work and patience!

@minrk minrk merged commit f81fb46 into jupyter:master Jul 11, 2017
@minrk
Copy link
Member

minrk commented Jul 11, 2017

Due to the issues with line endings and whitespace changes, I chose to squash this PR rather than deal with a complicated rebase. Hopefully we can catch similar issues earlier in subsequent PRs.

@gnestor gnestor mentioned this pull request Aug 3, 2017
Merged
@gnestor
Copy link
Contributor

gnestor commented Aug 7, 2017

Hi @JCEmmons! I'm about to publish a release candidate for notebook 5.1 and I noticed the following error when opening a new terminal (in the Jupyter Notebook):

image

Does this make sense to you?

@JCEmmons
Copy link
Contributor Author

JCEmmons commented Aug 7, 2017 via email

@takluyver
Copy link
Member

takluyver commented Aug 8, 2017 •
edited
Loading

I installed RC1 and I can't reproduce this - @gnestor did you get it worked out?

It does look like jed is listed in bower.json - maybe it was a matter of clearing out some cache?

@gnestor
Copy link
Contributor

gnestor commented Aug 8, 2017

I'm even seeing this in master 🤔 I don't see this when opening notebooks, just terminals.

@jcb91
Copy link
Contributor

jcb91 commented Sep 30, 2017

I'm seeing the same missing jed error breaking the jupyter_nbextensions_configurator standalone page at Jupyter-contrib/jupyter_nbextensions_configurator#43, using a pip-installed notebook 5.1.0 😢

jcb91 added a commit to jcb91/jupyter_nbextensions_configurator that referenced this pull request Oct 1, 2017
@jcb91
Apply an ugly patch for require config, needed for notebook 5.1
74615ad 
it got broken by jupyter/notebook#2140
@jcb91 jcb91 mentioned this pull request Oct 1, 2017
Merged
@JocelynDelalande JocelynDelalande mentioned this pull request Oct 21, 2017
Merged
@gnestor gnestor mentioned this pull request Dec 2, 2017
Closed
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Apr 5, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@takluyver takluyver takluyver left review comments

Assignees
No one assigned
Labels
status:resolved-locked tag:Accessibility
Projects
None yet
Milestone
5.1
Development

Successfully merging this pull request may close these issues.
5 participants
@JCEmmons @takluyver @minrk @gnestor @jcb91