Namespaced Javascript Catalog #41

afzaledx · 2018-05-11T16:58:49Z

This PR adds the namespace parameter to the command line, and can also be read from the STATICI18N_NAMESPACE

The concept behind the change is that the javascript catalog generated by django put the gettext against window.gettext. If, however, we have a modular system in which there are many pluggable modules that require the catalog, it does not make sense to combine the entire catalog into a single catalog. This becomes especially problematic for future modules that will be created.

This PR addresses the issue such that the generated javascript catalog is modified so that the gettext is put against window.NAMESPACE.gettext. The module can use the gettext from its own NAMESPACE rather than use the global namespace of the project.

Reviewers

(@zyegfryed)

coveralls · 2018-05-13T19:34:54Z

Coverage increased (+0.3%) to 96.35% when pulling bfc51e8 on open-craft:afzaledx/adding-namespace into fa65ede on zyegfryed:master.

pomegranited

I've tested this branch and it works as expected, and the added tests cover the feature. Just a few changes requested before I can approve.

Also: please bump the version in setup.py to e.g. 1.7.1 (Though note that the upstream maintainer may want a different version number).

I tested this using a virtualenv created for [MCKIN-7389-js-i18n] Adding i18n support in JS files openedx/xblock-drag-and-drop-v2#156
I read through the code
~~I checked for accessibility issues~~ N/A, command-line only
Includes documentation

pomegranited · 2018-05-14T22:42:19Z

docs/commands.rst

@@ -36,6 +36,14 @@ Some commonly used options are:

    Defaults to ``js``.

+``-n OUTPUT_FORMAT`` or ``--namespace=SpecialBlock``


nit: change OUTPUT_FORMAT to NAMESPACE

pomegranited · 2018-05-14T22:43:03Z

docs/settings.rst

+    and the namespaced contexts. The final gettext will be put under
+    window.<namespace>.gettext
+    rather than the
+    window.gettext


nit: fix word wrap

pomegranited · 2018-05-14T22:43:20Z

docs/settings.rst

+
+    :default: ``None``
+
+    The namespace under which the static javascipt will be put.


nit: capitalise javascript

pomegranited · 2018-05-14T22:44:04Z

src/statici18n/conf.py

@@ -15,3 +15,5 @@ class StaticFilesConf(AppConf):
    OUTPUT_DIR = 'jsi18n'
    # The dotted path to the function that creates the filename
    FILENAME_FUNCTION = 'statici18n.utils.default_filename'
+    # The namespace


Please expand this comment to explain what the namespace is used for. As it stands, the current comment adds nothing that the variable name doesn't already say.

pomegranited · 2018-05-14T22:45:03Z

src/statici18n/management/commands/compilejsi18n.py

+    def _get_namespaced_catalog(self, rendered_js, namespace):
+        template = u'''
+            (function(global){{
+                var {namespace}I18n = {{


Please remove the I18n suffix added here -- {namespace} should contain the full name of the var. As a developer, I want to be able to specify the full namespace used.

pomegranited · 2018-05-14T22:48:53Z

src/statici18n/management/commands/compilejsi18n.py

@@ -89,6 +107,7 @@ def _create_output(self, outputdir, outputformat, locale, domain, packages):

        if outputformat == 'js':
            data = self._create_javascript_catalog(locale, domain, packages)
+            data = self._get_namespaced_catalog(data, namespace) if namespace else data


Could you make this line clearer please?

if namespace: data = self._get_namespaced_catalog(data, namespace)

pomegranited · 2018-05-15T01:45:35Z

src/statici18n/management/commands/compilejsi18n.py

+                  init: function() {{
+                    {text}
+                  }}
+                }};


To make using this easier, could you please add this line right below here:

{namespace}.init()

pomegranited · 2018-05-15T10:20:52Z

docs/settings.rst

+
+    :default: ``None``
+
+    The namespace under which the static JAVASCRIPT will be put.


sorry @afzaledx -- I meant title case Javascript, not all caps 😄

afzaledx · 2018-05-15T11:14:09Z

@pomegranited, let me know if you have more suggestions.

pomegranited

@afzaledx Works well and looks good! 👍

Couple more little nits, but it's ready for upstream review once they're addressed, and your changes are squashed.

I tested this using the settings and translations for [MCKIN-7389-js-i18n] Adding i18n support in JS files openedx/xblock-drag-and-drop-v2#156.
I read through the code
~~I checked for accessibility issues~~ N/A, command line only
Includes documentation

pomegranited · 2018-05-15T23:10:05Z

docs/settings.rst

+    This is useful for pluggable modules which need Javascript i18n.
+
+
+


nit: remove empty lines at end of file

pomegranited · 2018-05-15T23:10:51Z

src/statici18n/conf.py

@@ -15,3 +15,5 @@ class StaticFilesConf(AppConf):
    OUTPUT_DIR = 'jsi18n'
    # The dotted path to the function that creates the filename
    FILENAME_FUNCTION = 'statici18n.utils.default_filename'
+    # The namespace under which the static JAVASCRIPT will be put.


nit: Javascript, not JAVASCRIPT

pomegranited · 2018-05-15T23:11:07Z

src/statici18n/management/commands/compilejsi18n.py

@@ -49,6 +49,25 @@ def add_arguments(self, parser):
                            default='js',
                            help="Format of the output catalog. "
                            "Options are: js, json. Defaults to js.")
+        parser.add_argument('-n', '--namespace', dest='namespace',
+                            metavar='NAMESPACE', default=settings.STATICI18N_NAMESPACE,
+                            help="value for the namespace. "


nit: remove trailing space

nit: "Javascript identifier to use as namespace" ?

If a namespace is provided, the gettext is assigned to window.namespace.gettext instead of window.gettext.

afzaledx · 2018-05-16T11:43:23Z

@zyegfryed, This is PR is ready for your review.

zyegfryed · 2018-05-16T12:06:08Z

@afzaledx @pomegranited Thanks for your work! I'll have a look this week-end.

pomegranited · 2018-05-22T01:11:30Z

Hi @zyegfryed -- sorry to bother you, but we were wondering if you needed any changes made here? We've got time allocated this week and next if there's anything we can do?

zyegfryed · 2018-05-22T08:56:26Z

Hi @pomegranited , I'm sorry but I haven't have a look into the PR, yet :( Will try tomorrow evening. Cheers

pomegranited · 2018-05-22T23:55:50Z

No worries @zyegfryed ! Thank you for your time.

zyegfryed · 2018-05-31T17:09:15Z

@afzaledx @pomegranited I released v1.8.0 with your patch.
Thanks 🍰

afzaledx · 2018-05-31T17:22:12Z

@zyegfryed, You are welcome, and thank you for incorporating our contribution.

afzaledx force-pushed the afzaledx/adding-namespace branch 2 times, most recently from f8713b5 to 8276082 Compare May 13, 2018 19:31

afzaledx force-pushed the afzaledx/adding-namespace branch from fab34a6 to 48de5cd Compare May 14, 2018 13:26

afzaledx changed the title ~~(WIP) added the namespace parameter~~ Namespaced Javascript Catalog May 14, 2018

pomegranited mentioned this pull request May 15, 2018

Namespacing #40

Closed

pomegranited suggested changes May 15, 2018

View reviewed changes

pomegranited reviewed May 15, 2018

View reviewed changes

pomegranited mentioned this pull request May 15, 2018

[MCKIN-7389-js-i18n] Adding i18n support in JS files openedx/xblock-drag-and-drop-v2#156

Merged

2 tasks

pomegranited reviewed May 15, 2018

View reviewed changes

pomegranited approved these changes May 15, 2018

View reviewed changes

pomegranited added a commit to open-craft/xblock-drag-and-drop-v2 that referenced this pull request May 15, 2018

Using branch from zyegfryed/django-statici18n#41

d2b4e04

Added the namespace parameter.

bfc51e8

If a namespace is provided, the gettext is assigned to window.namespace.gettext instead of window.gettext.

afzaledx force-pushed the afzaledx/adding-namespace branch from f9593d2 to bfc51e8 Compare May 16, 2018 11:38

zyegfryed merged commit 0574b34 into zyegfryed:master May 31, 2018

pomegranited deleted the afzaledx/adding-namespace branch June 1, 2018 03:31

pomegranited mentioned this pull request Jul 31, 2018

Restructure i18n files open-craft/problem-builder#200

Merged

bradenmacdonald mentioned this pull request Aug 29, 2018

i18n tooling, JavaScript i18n and added the Japanese language open-craft/xblock-poll#43

Merged

5 tasks

This was referenced Sep 2, 2018

i18n tooling + translation updates oppia/xblock#2

Closed

i18n tooling + translations appsembler/oppia-xblock#1

Closed

i18n tooling and translation openedx/xblock-google-drive#27

Merged

[WRONG PR, PLEASE IGNORE] i18n tooling with Transifex oppia/xblock#3

Closed

OmarIthawi mentioned this pull request Sep 6, 2018

i18n tooling using Transifex appsembler/oppia-xblock#2

Merged

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Namespaced Javascript Catalog #41

Namespaced Javascript Catalog #41

afzaledx commented May 11, 2018 •

edited by zyegfryed

Loading

coveralls commented May 13, 2018 •

edited

Loading

pomegranited left a comment

pomegranited May 14, 2018

pomegranited May 14, 2018

pomegranited May 14, 2018

pomegranited May 14, 2018

pomegranited May 14, 2018

pomegranited May 14, 2018

pomegranited May 15, 2018

pomegranited May 15, 2018

afzaledx commented May 15, 2018

pomegranited left a comment

pomegranited May 15, 2018

pomegranited May 15, 2018

pomegranited May 15, 2018

pomegranited May 15, 2018

afzaledx commented May 16, 2018

zyegfryed commented May 16, 2018

pomegranited commented May 22, 2018

zyegfryed commented May 22, 2018

pomegranited commented May 22, 2018

zyegfryed commented May 31, 2018

afzaledx commented May 31, 2018

		@@ -36,6 +36,14 @@ Some commonly used options are:

		Defaults to ``js``.

		``-n OUTPUT_FORMAT`` or ``--namespace=SpecialBlock``


		:default: ``None``

		The namespace under which the static javascipt will be put.

		This is useful for pluggable modules which need Javascript i18n.

Namespaced Javascript Catalog #41

Namespaced Javascript Catalog #41

Conversation

afzaledx commented May 11, 2018 • edited by zyegfryed Loading

coveralls commented May 13, 2018 • edited Loading

pomegranited left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

afzaledx commented May 15, 2018

pomegranited left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

afzaledx commented May 16, 2018

zyegfryed commented May 16, 2018

pomegranited commented May 22, 2018

zyegfryed commented May 22, 2018

pomegranited commented May 22, 2018

zyegfryed commented May 31, 2018

afzaledx commented May 31, 2018

afzaledx commented May 11, 2018 •

edited by zyegfryed

Loading

coveralls commented May 13, 2018 •

edited

Loading