Don't merge obsolete entity fields

RESOLVED FIXED

Status

defect
P2
normal
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: zbraniecki, Assigned: zbraniecki)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

Our fairly dummy l10n-merge in FirefoxOS is currently merging whole entities irrelevant of their fields.

That causes problems when the obsolete entity in ab-CD contains different fields than new en-US.

A solution would be to look into entity and copy only the fields (value/attributes) that are in source locale.
Priority: -- → P2
Posted file pull request
So, what this patch does is that it changes how we do our naive l10n-merge.

With it, we will look into the top-level structure of each entity and verify if it has the same fields. If the structure of the entity is different, we'll pick the source entity.

That also matches closer to how it will work in l20n where we will have the concept of malformed entity (entity with public fields not matching source entity).

For now, it will fix the problem of obsolete entities with values leaking into result locales and causing UI regressions.
Assignee: nobody → gandalf
Status: NEW → ASSIGNED
Attachment #8513942 - Flags: review?(stas)
Attachment #8513942 - Flags: feedback?(l10n)
Comment on attachment 8513942 [details] [review]
pull request

I commented in the PR, although pretty please attach textual diffs in Bugzilla in the future.  r=me with comments addressed.
Attachment #8513942 - Flags: review?(stas) → review+
Comment on attachment 8513942 [details] [review]
pull request

Thanks Stas for the review!
Attachment #8513942 - Flags: feedback?(l10n)
I landed this in the l20n.js repo as well:

https://github.com/l20n/l20n.js/commit/c81d1981d74708e5d8e56c825fc996c6b777304b

Zibi, I thought you were going to land the version with keys1.every() ?
Flags: needinfo?(gandalf)
(In reply to Staś Małolepszy :stas from comment #5)
> I landed this in the l20n.js repo as well:
> 
> https://github.com/l20n/l20n.js/commit/
> c81d1981d74708e5d8e56c825fc996c6b777304b

Oh, thanks!

> Zibi, I thought you were going to land the version with keys1.every() ?

No, since we decided not to go for two loops, but rather length match plus one loop I don't think that an external function makes sense and inline loop does not inflate the stack.
Flags: needinfo?(gandalf)
I agreed with you not to do (keys1.every && keys2.every).  However, I thought we said keys1.every was okay.  It's one loop, a much cleaner functional approach and is only one line rather than 3 which leak the implementation of for-loop.
Anyways, this had my r+, so it's okay, I just wanted to clarify my position and my preference on the style.  I do think keys1.every is easier to read and we're not even remotely close to care about the call stack here.
Yeah, I'm afraid that it's a difference in our preferences - imperative vs. declarative.

Yours seems to be driven by code readability, mine on CPU cycles, performance [1], and memory.

[1] http://jsperf.com/every-vs-loop222
You need to log in before you can comment on or make changes to this bug.