1222055 - Update l20n parsers (js+python) to error on not-resolvable values

Status: RESOLVED FIXED

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

Description

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

A conclusion from bug 1199670 comment 8 and bug 1199670 comment 9 is that we want to make l20n parser error when a value is not resolvable.

Comment 1

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

Attachment: pull request

First, JS patch.

Comment 2

[Axel Hecht [:Pike]]

I looked at the patch a bit, and I found the boolean logic hard to understand.

There's a not equals undefined which gets negated into optional which means it could be missed.

That's like how many negations?

Maybe the code is easier to follow if it's using hasIndex and isRequired ?

I'm generally torn on the readability of the three optional args.

Lastly, it doesn't seem that the failure cases here got tests?

Comment 3

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

> Maybe the code is easier to follow if it's using hasIndex and isRequired ?

Named them :)

> Lastly, it doesn't seem that the failure cases here got tests?

Yup it's there, as part of hash errors test now.

Comment 4

[Staś Małolepszy :stas]

I'm with Pike on the boolean logic here.  I wonder if isRequired (which I'd rename to valueRequired) is needed at all?

Comment 5

[Staś Małolepszy :stas]

Comment on attachment 8685525 [details] [review]
pull request

Thanks for addressing the review comments.  r=me with one additional nit:  perhaps rename the arguments to getValue to `hasIndex` and `isRequired`?

Comment 6

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

Stas, so I looked at the code in parsers and I don't use the convention that you are suggesting - names of boolean variables do not follow "is*" or "has*".

If you're ok with not enforcing a style switch here, I'd prefer to land it in sync with the current code style in those files.

Comment 7

[Staś Małolepszy :stas]

Yeah, absolutely, go for it!

Comment 8

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

JS landed: https://github.com/l20n/l20n.js/commit/d70a9ccd9ea908b0b4b8be375808b31a20d31c23

Keeping open for python patch

Comment 9

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

Python patch: https://github.com/l20n/python-l20n/commit/25100ef5d78991c960c702481e8a342f85e4a22c

Pike, can you test it and confirm if it works for you?

Comment 10

[Axel Hecht [:Pike]]

LGTM. I'm a tad torn on the mix between boolean and None for index, but I figured that out by now, I think.