2294 pontoon to webextension #2358

jaredhirsch · 2017-03-11T00:00:04Z

This is built on top of the string extraction commit from #2344, which hasn't landed yet.

Commit message pretty much says it all:

Add pontoon strings & build step to transform into webextension strings

Add pontoon-to-webext.js script from bwinton/SnoozeTabs repo and
related npm dependencies (to be removed when/if that script is
published on npm as a standalone module).
Add extracted strings to a properties file at the location expected by
Pontoon.
Add a pontoon-to-webextension build step to the Makefile
Also, replace the hand made messages.json with the more nicely-formatted
script output.

This commit, together with the fix for #2344, closes #2294.

jaredhirsch · 2017-03-11T00:00:49Z

Makefile

@@ -35,6 +35,9 @@ imgs_server_dest := $(imgs_source:%=build/server/%)

 raven_source := $(shell node -e 'console.log(require.resolve("raven-js/dist/raven.js"))')

+l10n_source := $(wildcard locales/*/*.properties)
+l10n_dest := $(l10n_source:%/*.properties=addon/webextension/_locales/%/messages.json)


not sure about this, but make doesn't seem to complain, so I guess it's alright?

This doesn't look right to me, but I'm not sure. What should the value of $(l10n_source) be? If it's like locales/en-US/something.properties locales/es-ES/something.properties and you want to get addon/webextensions/_locales/en-US/messages.json addon/webextensions/_locales/en-US/messages.json then it's right. This presumes that each directory has only one properties file and that it's name isn't important.

I was thinking we'd eventually have multiple properties files in this directory, hence the double wildcard. However, now that I think about it, we would only ever want to package up the addon's strings in the addon...so I'll update this to grab $(wildcard locales/*/webextension.properties)

jaredhirsch · 2017-03-11T00:03:26Z

@flodolo Hi, here's the other localization step, with the strings translated into Pontoon-ready .properties format. Comments again appreciated. The pontoon-to-webext script from snooze tabs worked perfectly, thanks for the suggestion 👍 (and huge thanks to @lmorchard for writing such a great tool).

jaredhirsch · 2017-03-11T00:27:51Z

Makefile

@@ -35,6 +35,9 @@ imgs_server_dest := $(imgs_source:%=build/server/%)

 raven_source := $(shell node -e 'console.log(require.resolve("raven-js/dist/raven.js"))')

+l10n_source := $(wildcard locales/*)
+l10n_dest := $(l10n_source:%/webextension.properties=addon/webextension/_locales/%/messages.json)


@ianb meh, decided to just solve today's problem, and worry about building server strings when we get there

flodolo

One tiny update to a comment but it looks great

flodolo · 2017-03-11T06:07:42Z

locales/en-US/webextension.properties

+cancelScreenshot = Cancel
+downloadScreenshot = Download
+notificationLinkCopiedTitle = Link Copied
+# LOCALIZATION NOTE(notificationLinkCopiedDetails): The string "{meta_key}-V" should be translated


You can avoid the LOCALIZATION NOTE part, since tools will associate and display the comment to the following string.

Possible comment:

# The string "{meta_key}-V" should be translated to the region-specific # shorthand for the Paste keyboard shortcut. {meta_key} is a placeholder for the # modifier key used in the shortcut (do not translate it): for example, Ctrl-V # on Windows systems.

👍 Thanks for the feedback! I'll update that comment.

jaredhirsch · 2017-03-12T15:11:11Z

@ianb Is this good to land?

ianb · 2017-03-13T18:50:05Z

Makefile

@@ -103,6 +106,9 @@ signed_xpi: addon
 	./node_modules/.bin/web-ext sign --api-key=${AMO_USER} --api-secret=${AMO_SECRET} --source-dir addon/webextension/
 	mv web-ext-artifacts/*.xpi build/pageshot.xpi

+addon_locales: $(l10n_dest)


I'm not confident exactly how this will work. Generally the Makefile pattern would be to set the requirement and then have other rules for how to generate addon/webextension/_locales/%/messages.json, like:

addon/webextension/_locales/%/messages.json: locales/%/webextension.properties script-that-translates-one-file $< $@

Since this appears to generate all the files in one go, it doesn't quite fit that pattern. I can't decide if $(l10n_source) belongs here, or maybe not, or maybe nothing at all?

Also there should be .PHONY: addon_locales above this

Upon further reflection, I guess an empty dependencies list is the right thing here, given that we don't have recipes for how to generate those individual files. Either way I believe the .json files get rebuilt on every make run because we're not telling make enough for it to know when a rebuild is necessary.

👍 The rebuild is pretty cheap, for now at least (one locale...)

ianb · 2017-03-13T18:50:24Z

Makefile

@@ -103,6 +106,9 @@ signed_xpi: addon
 	./node_modules/.bin/web-ext sign --api-key=${AMO_USER} --api-secret=${AMO_SECRET} --source-dir addon/webextension/
 	mv web-ext-artifacts/*.xpi build/pageshot.xpi

+addon_locales: $(l10n_dest)
+	./bin/pontoon-to-webext.js --dest addon/webextension/_locales


pontoon-to-webext should go in bin/build-scripts/ since it isn't called directly, instead it's only called through the Makefile

ianb · 2017-03-13T18:52:01Z

package.json

@@ -16,8 +16,10 @@
    "envc": "2.5.0",
    "escape-html": "1.0.3",
    "express": "4.15.2",
+    "habitat": "^3.1.2",


We've generally been using exact versions, then upgrading in batch, so that we know everyone has the exact same version.

ianb · 2017-03-13T18:52:35Z

addon/webextension/_locales/en_US/messages.json

@@ -1,33 +1,79 @@
 {


Should this file be in the source at all? Isn't it generated from the properties file?

👍 removed

er, removed and gitignore'd

ianb · 2017-03-13T18:54:17Z

bin/pontoon-to-webext.js

+          const langcode = localeLocation.split(path.sep).slice(-1)[0];
+
+          if (langcode) {
+            localeList.push(langcode);


Why wouldn't this be present? Seems like it should be a warning.

Hmm. I agree that it's hard to see how langcode could fail to exist.

ianb · 2017-03-13T18:55:43Z

bin/pontoon-to-webext.js

+        dirTree.splice(0,1);
+        dirTree.forEach((localeLocation) => {
+          // Get the locale code from the end of the path. We're expecting the structure of Pontoon's output here
+          const langcode = localeLocation.split(path.sep).slice(-1)[0];


Isn't localeLocation.split(path.sep).slice(-1)[0] equivalent to localeLocation.split(path.sep)[1] ? That is, split with a string argument always splits 0 or 1 times.

(I find that split behavior confusing, so I'm always suspicious when I see it being used.)

(To be clear, this is upstream code that I'm hoping Les will turn into an npm package. We're using copypasta because it's expedient.)

So, this line is getting the last directory's name off an absolute path; the slice(-1)[0] is effectively the same as doing a pop() off the end of the array.

let localeLocation = '/Users/jhirsch/codez/github/mozilla-services-pageshot/locales/en-US' localeLocation.split('/') // Array [ "", "Users", "jhirsch", "codez", "github", "mozilla-services-pageshot", "locales", "en-US" ] localeLocation.split('/').slice(-1) // Array [ "en-US" ] localeLocation.split('/').slice(-1)[0] // "en-US"

ianb · 2017-03-13T18:56:33Z

bin/pontoon-to-webext.js

+        reject(e);
+      });
+    } else {
+      supportedLocales = supportedLocales.split(',').map(item => item.trim());


Probably should be supportedLocales.split(/,/g)

I think we're good here, since split splits globally by default:

'a,b,c'.split(',') // returns [ "a", "b", " c" ]

ianb · 2017-03-13T19:00:07Z

bin/pontoon-to-webext.js

+    }).then(() => {
+      log(`Done compiling locales at: ${localesPath}`);
+    }).catch((e) => {
+      error(e);


These errors won't lead to a build error, but they should. It's okay if build scripts are a little finicky as long as they fail hard.

I'd recommend:

function fatal(...args) { console.error(...args); process.exit(1); }

Also this is all kind of brutal in Node.js compared to Python, but eh.

👍 changed (I went ahead and upstreamed that suggestion :-)

- Add pontoon-to-webext.js script from bwinton/SnoozeTabs repo and related npm dependencies (to be removed when/if that script is published on npm as a standalone module). - Add extracted strings to a properties file at the location expected by Pontoon. Remove the webextension-formatted strings from git. - Add a pontoon-to-webextension build step to the Makefile. This commit, together with the fix for mozilla-services#2344, closes mozilla-services#2294.

jaredhirsch · 2017-03-14T02:28:45Z

@ianb Updated. Mind taking another look?

jaredhirsch commented Mar 11, 2017

View reviewed changes

jaredhirsch force-pushed the 2294-pontoon-to-webextension branch 2 times, most recently from 69de1cb to 2d123da Compare March 11, 2017 00:27

jaredhirsch commented Mar 11, 2017

View reviewed changes

jaredhirsch force-pushed the 2294-pontoon-to-webextension branch 3 times, most recently from 3224cfe to cf3d02c Compare March 11, 2017 01:21

flodolo approved these changes Mar 11, 2017

View reviewed changes

jaredhirsch force-pushed the 2294-pontoon-to-webextension branch from cf3d02c to 063c2b4 Compare March 12, 2017 15:10

ianb suggested changes Mar 13, 2017

View reviewed changes

jaredhirsch force-pushed the 2294-pontoon-to-webextension branch from 063c2b4 to cc3cd1c Compare March 14, 2017 02:24

jaredhirsch force-pushed the 2294-pontoon-to-webextension branch from cc3cd1c to f9e24d7 Compare March 14, 2017 02:25

jaredhirsch requested a review from ianb March 14, 2017 02:28

ianb mentioned this pull request Mar 14, 2017

Pr/2358 #2375

Merged

ianb merged commit f9e24d7 into mozilla-services:master Mar 14, 2017

jaredhirsch deleted the 2294-pontoon-to-webextension branch March 14, 2017 16:50

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

2294 pontoon to webextension #2358

2294 pontoon to webextension #2358

jaredhirsch commented Mar 11, 2017

jaredhirsch Mar 11, 2017

ianb Mar 11, 2017

jaredhirsch Mar 11, 2017

jaredhirsch commented Mar 11, 2017

jaredhirsch Mar 11, 2017

flodolo left a comment

flodolo Mar 11, 2017

jaredhirsch Mar 12, 2017

jaredhirsch commented Mar 12, 2017

ianb Mar 13, 2017

ianb Mar 13, 2017

jaredhirsch Mar 14, 2017

ianb Mar 13, 2017

jaredhirsch Mar 14, 2017

ianb Mar 13, 2017

jaredhirsch Mar 14, 2017

ianb Mar 13, 2017

jaredhirsch Mar 14, 2017

jaredhirsch Mar 14, 2017

ianb Mar 13, 2017

jaredhirsch Mar 14, 2017

ianb Mar 13, 2017

jaredhirsch Mar 14, 2017

ianb Mar 13, 2017

jaredhirsch Mar 14, 2017

ianb Mar 13, 2017

jaredhirsch Mar 14, 2017

jaredhirsch commented Mar 14, 2017

2294 pontoon to webextension #2358

2294 pontoon to webextension #2358

Conversation

jaredhirsch commented Mar 11, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jaredhirsch commented Mar 11, 2017

Choose a reason for hiding this comment

flodolo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jaredhirsch commented Mar 12, 2017

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

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

jaredhirsch commented Mar 14, 2017