940772 - Dehardcode plural on Gaia branch

Status: RESOLVED FIXED

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

Description

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

We currently do several things that hardcode plural macro on Gaia branch.

I'd like to add formFactor support so I need to fix that.

There are two elements:

1) parser hardcodes plural
2) compiler hardcode single arg macro calls

Both should be relatively easy to fix.

Comment 1

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

I landed 1) https://github.com/l20n/l20n.js/commit/0a3ab8b6c4171dd9bfbd088eedffb15b4d942890

Comment 2

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

Attachment: POC

This is a POC of the compiler work.

Stas: Can you take a look at this POC and review it for me?

Thanks!

Comment 3

[Staś Małolepszy :stas]

Comment on attachment 8334972 [details] [diff] [review]
POC

Review of attachment 8334972 [details] [diff] [review]:
-----------------------------------------------------------------

It looks good, although I didn't have time to test in myself yet.  AIUI, the main goal here is to allow for call expressions to not have any arguments, and the approach seems fine.

::: lib/l20n/compiler.js
@@ +402,5 @@
>        var callee = Expression(node.callee, entry);
>        // support only one argument per callExpr for now
> +      var arg = null;
> +      if (node.arguments.length) {
> +        arg = Expression(node.arguments[0], entry);

I think you shouldn't need to do this here, because the Expression constructor will return null if node.arguments[0] is falsy.

@@ +419,2 @@
>  
> +          // special cases for zero, one, two if they are defined on the hash

Should we leave those special cases here untouched, or should we make them plural-specific?

I'm fine with leaving them as they are.

Comment 4

[Staś Małolepszy :stas]

Attachment: Make indexes accept various expressions

The old POC was based on the old branch of l10n.js.

This patch makes it possible to use any kind of expression in the index:
 - a macro call with an argument: {[ plural(n) ]}
 - a macro call without arguments: {[ macro() ]}
 - a string or variable identifier: {[ brandName ]}

I probably need more tests, but the logic in the patch is good to review at this point.

Comment 5

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

Comment on attachment 8446114 [details] [diff] [review]
Make indexes accept various expressions

Review of attachment 8446114 [details] [diff] [review]:
-----------------------------------------------------------------

Sorry it took me so long Stas! The patch looks good, although I'm not sure how much this will change when we add l20n parser.

Comment 6

[Staś Małolepszy :stas]

These changes will make it easier to add a subset of the l20n syntax, so this should be a change into the right direction.  I submitted a pull request, waiting for TBPL before merging:

https://github.com/mozilla-b2g/gaia/pull/23806
https://tbpl.mozilla.org/?rev=70462fa574e5&tree=Gaia-Try

Comment 7

[Staś Małolepszy :stas]

Commit: https://github.com/mozilla-b2g/gaia/commit/b9385cb197f275e43b087ada82dc5fcfe85bcb9d
Master: https://github.com/mozilla-b2g/gaia/commit/5110fc938f26acbee951df8233412b8001cdf357

L20n.js: https://github.com/l20n/l20n.js/commit/6dc19ec1d83ba9ce137b570d569ba5758c0baf71