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

Fix moment locale loading in bidi support #3048

Merged
merged 2 commits into from
Nov 16, 2017
Merged

Fix moment locale loading in bidi support #3048

merged 2 commits into from
Nov 16, 2017

Conversation

jcb91
Copy link
Contributor

@jcb91 jcb91 commented Nov 16, 2017

This is throwing errors like

Uncaught Error: Script error for "components/moment/locale/en-gb"
http://requirejs.org/docs/errors.html#scripterror
    at makeError (require.js?v=6da8be361b9ee26c5e721e76c6d4afce:165)
    at HTMLScriptElement.onScriptError (require.js?v=6da8be361b9ee26c5e721e76c6d4afce:1732)
makeError @ require.js?v=6da8be361b9ee26c5e721e76c6d4afce:165
onScriptError @ require.js?v=6da8be361b9ee26c5e721e76c6d4afce:1732

because of the way it's been implemented (issuing a require call to a presumed locale js file, which may not exist, especially since packaged versions of notebook are shipping with minimized moment, without the separate locale files). This PR does it in the correct way (getting moment to load it from its own internal set, if possible, also falling back to less-specific versions as necessary).

this is the correct way to do it, especially since packaged versions of notebook are shipping without separate locale files
remove unused variable, redundant comment, don't need jquery
@jcb91
Copy link
Contributor Author

jcb91 commented Nov 16, 2017

Also, while looking at this, I've noticed that

  • there are quite a few country-specific arabic locales (e.g. ar-sa), but the current check for whether to use the numericshaping at static/bidi/bidi.js#L29 is checking for the exact string ar, which means it'll decide not to map them. This might be intentional, but I suspect it isn't?

  • moment's arabic locales already provide functions for transforming numbers in strings (see, e.g. moment/src/locale/ar-sa.js#L85-L89). Perhaps it might make more sense to use these for the numericshaping functionality than rolling new ones in notebook? This might make the above check redundant too, we could instead do something like

    var shapeNumerals = moment.localeData().postformat;
    var applyBidi = function (value) {
        return shapeNumerals(value);
    };

@takluyver
Copy link
Member

Thanks, I don't know much about this, but this looks like an improvement to me.

cc @samarsultan and @rgbkrk (you've touched this file)

@takluyver takluyver added this to the 5.3 milestone Nov 16, 2017
@takluyver
Copy link
Member

I think you're right about the exact ar check, as well - it should be checking if it starts with that.

Copy link
Member

@rgbkrk rgbkrk left a comment

Choose a reason for hiding this comment

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

Yeah this is much better to let moment resolve it.

It would be good to verify this with a non-English locale.

@takluyver
Copy link
Member

I've done a brief test with an arabic locale set. I made and installed an sdist to check for packaging problems. It seemed to be working, whereas 5.2.1 installed through conda still showed me 'ago' times in English, and had the error about loading the locale.

@rgbkrk rgbkrk merged commit b3b8106 into jupyter:master Nov 16, 2017
@takluyver takluyver mentioned this pull request Nov 16, 2017
@takluyver takluyver mentioned this pull request Dec 12, 2017
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Apr 3, 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.

3 participants