1013831 - Detect duplicated strings in .properties file and make Travis fail

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Gaia::L10n, defect, P3)

Description

[Francesco Lodolo [:flod]]

Discussed about this on IRC with :stas

Target is to avoid bugs like bug 1009351, where a duplicated string ID is added to the .properties file: this makes tools unhappy, and let us display the override value.

It should possible to do this in l10n.js

Comment 1

[Francesco Lodolo [:flod]]

(In reply to Francesco Lodolo [:flod] from comment #0)
> Target is to avoid bugs like bug 1009351

Bug 1013826 is about removing the string, that is the implementation bug which is going to actually use the string.

Comment 2

[Staś Małolepszy :stas]

This turns out to be slightly more involved:

- We don't want to have this logic in the parser, because a) it doesn't belong there, b) parsing is designed to be merciful: errors are emitted, not thrown, and parsing continues uninterrupted.  I'd prefer to have this logic in Locale.addAST.

- Currently, however, the parser produces an AST which already is a dict, so any duplicate strings are overwritten during the parsing stage.  It would generally be good for the AST to be a list, and I filed bug 1020231 for this.

- If we throw an error in Locale.addAST, the context will never become ready.  I think the build will actually complete fine, but the app will be broken.  As in, all integration tests are likely to fail. This might be a bit drastic :)

Comment 3

[Francesco Lodolo [:flod]]

We were hit again in bug 951538. Any progress on this?

Comment 4

[Staś Małolepszy :stas]

We're going to factorize the parser in bug 1022859.  Next, I'll work on bug 1020231 which should make it possible to move forward with this bug.  I'll have another update next week.

Comment 5

[Staś Małolepszy :stas]

WIP: https://github.com/stasm/gaia/tree/1013831-detect-duplicate-strings

Comment 6

[Staś Małolepszy :stas]

I think I got this to work.  I'll now need to remove the duplicates which break the build :)

Most of them is easy as they redefine strings from shared/locales/date and shared/locales/download.

Communications is a bit harder since the strings in Dialer are used in Contacts and the other way around.  I think I'll file a separate bug for this.

/build_stage/emergency-call/index.html: L10nError: Duplicate string "cancel" found in app://emergency-call.gaiamobile.org
/build_stage/sms/index.html: L10nError: Duplicate string "incorrectDate" found in app://sms.gaiamobile.org
/build_stage/system/index.html: L10nError: Duplicate string "byteUnit-B" found in app://system.gaiamobile.org
/build_stage/system/index.html: L10nError: Duplicate string "byteUnit-KB" found in app://system.gaiamobile.org
/build_stage/system/index.html: L10nError: Duplicate string "byteUnit-MB" found in app://system.gaiamobile.org
/build_stage/system/index.html: L10nError: Duplicate string "byteUnit-GB" found in app://system.gaiamobile.org
/build_stage/system/index.html: L10nError: Duplicate string "byteUnit-TB" found in app://system.gaiamobile.org
/build_stage/system/index.html: L10nError: Duplicate string "fileSize" found in app://system.gaiamobile.org
/build_stage/communications/dialer/index.html: L10nError: Duplicate string "call" found in app://communications.gaiamobile.org
/build_stage/communications/dialer/index.html: L10nError: Duplicate string "edit" found in app://communications.gaiamobile.org
/build_stage/communications/dialer/index.html: L10nError: Duplicate string "cancel" found in app://communications.gaiamobile.org
/build_stage/communications/dialer/index.html: L10nError: Duplicate string "contacts" found in app://communications.gaiamobile.org
/build_stage/communications/dialer/index.html: L10nError: Duplicate string "selectAll" found in app://communications.gaiamobile.org
/build_stage/communications/dialer/index.html: L10nError: Duplicate string "deselectAll" found in app://communications.gaiamobile.org
/build_stage/communications/dialer/index.html: L10nError: Duplicate string "delete" found in app://communications.gaiamobile.org
/build_stage/communications/dialer/index.html: L10nError: Duplicate string "makeCall" found in app://communications.gaiamobile.org
/build_stage/communications/dialer/index.html: L10nError: Duplicate string "today" found in app://communications.gaiamobile.org
/build_stage/communications/dialer/index.html: L10nError: Duplicate string "yesterday" found in app://communications.gaiamobile.org
/build_stage/communications/dialer/index.html: L10nError: Duplicate string "cancel" found in app://communications.gaiamobile.org
/build_stage/settings/index.html: L10nError: Duplicate string "byteUnit-B" found in app://settings.gaiamobile.org
/build_stage/settings/index.html: L10nError: Duplicate string "byteUnit-KB" found in app://settings.gaiamobile.org
/build_stage/settings/index.html: L10nError: Duplicate string "byteUnit-MB" found in app://settings.gaiamobile.org
/build_stage/settings/index.html: L10nError: Duplicate string "byteUnit-GB" found in app://settings.gaiamobile.org
/build_stage/settings/index.html: L10nError: Duplicate string "byteUnit-TB" found in app://settings.gaiamobile.org

Comment 7

[Staś Małolepszy :stas]

I filed bug 1098673 for Communications and fixed other apps on my branch.

Comment 8

[Staś Małolepszy :stas]

Attachment: Throw on duplicate strings

flod, can you take a look at the changes in shared/locales/download and let me know if it's okay to just move these strings there from System and Settings?

Before a proper review request, I want to add build integration tests.  I'll combine this with my work in bug 928740 and try to have it done by the end of next week.

Comment 9

[Francesco Lodolo [:flod]]

Comment on attachment 8522631 [details] [review]
Throw on duplicate strings

Moved strings look definitely good to me, especially B instead of Bytes.
We should probably drop the comment about fileSize*

Comment 10

[Staś Małolepszy :stas]

Flod, do I need to change the id of the byteUnit-B string? :/

Comment 11

[Francesco Lodolo [:flod]]

Absolutely no. It's in a different file, so it will be re-translated anyway.

Comment 12

[Staś Małolepszy :stas]

Comment on attachment 8522631 [details] [review]
Throw on duplicate strings

https://treeherder.mozilla.org/#/jobs?repo=gaia-try&revision=a3b0de18f1e4

Comment 13

[Zibi Braniecki [:zbraniecki][:gandalf]]

Comment on attachment 8522631 [details] [review]
Throw on duplicate strings

I'm a little worried about the scope of changes you introduce here, but since it's early in the cycle and I like all of them, r+ :)

I left some notes about eventlisteners that seem abundant.

Comment 14

[GitHub Autolander Bot]

https://github.com/mozilla-b2g/gaia/pull/25451

The pull request could not be applied to the integration branch. Please try again after current integration is complete. You may need to rebase your branch against the target branch.

Comment 15

[Staś Małolepszy :stas]

Master: https://github.com/mozilla-b2g/gaia/commit/d6cf6dde876c897a68c18ed95cad3bd506467ce4

Comment 16

[Staś Małolepszy :stas]

L20n.js: https://github.com/l20n/l20n.js/commit/27af1c9b595d38be7a0eb783099cf1b2584a026d