warn about duplicated properties

[matkoniecz]

tangram play validator may complain about things like

stroke: { color: black, width: 1 }
stroke: { color: global.text_stroke, width: [[1,1px], [12,2px],[16,4px]] }

(real map style bug, fixed in matkoniecz/Zazolc@d1bc84f)

related to tangrams/tangram-es#1584

[matteblair]

This issue reaches into Tangram JS itself - Tangram Play doesn't do much in the way of validating the scene file. @meetar Do you know whether Tangram JS can issue a warning for duplicated mapping entries like this?

[meetar]

Yes, I think it would be possible, but I'm not sure I'd support the inclusion of this kind of feature without some way of making it optional, because there may be cases where this kind of duplication is intentional, and of course there's no way for the parser to know this. For instance: if one of the declarations is in an imported file, it may be intended to override the base scene.

Maybe some kind of 'verbosity' parameter?

[matteblair]

If we care about scene file portability, then repeated mapping keys should always be discouraged if not forbidden. Different YAML parsers (or even different versions of the same parser) can treat repeated keys differently because there is no correct way to handle them! That means that you could write a scene file with repeated keys in Tangram Play, with everything working and looking just the way you want, then load it in Tangram ES and see broken styling with no warning! Then to fix it you have to 1. know that this repeated-key problem exists, 2. manually locate a repeated key somewhere in your scene file, and 3. figure out how Tangram Play is handling the repeated key so you can remove the unused key.

[meetar]

That's true, for some reason I was only considering post-merging – I'm not sure whether Tangram JS currently does its parsing first. If the merge happens first then afaik without some kind of source map equivalent it won't be possible to detect whether duplicate keys occur within a single file or not, and though I assume parsing first is possible there may be performance implications.

Tagging @bcamper for feedback when he's back next week.

[matteblair]

Ah I see, yeah this would only occur in the parsing step before any merging occurs. The parsing step will choose one of the key-value pairs to use and then drop the other, and we don't know which it will pick.

[bcamper]

We've looked at this before but I believe js-yaml doesn't support any kind of introspection for this kind of thing.

…

On Mon, Jul 24, 2017 at 1:02 PM Matt Blair ***@***.***> wrote: Ah I see, yeah this would only occur in the parsing step before any merging occurs. The parsing step will choose one of the key-value pairs to use and then drop the other, and we don't know which it will pick. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#743 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AABBXd8LRhOiFqYFWdFV2OcJZPnPS-9tks5sRPhogaJpZM4OgaBD> .

[matteblair]

This might be supported by js-yaml? It has something that looks like a custom error handler: nodeca/js-yaml@e6bac15

[meetar]

Just ran a test with the js-yaml CLI on this yaml file:

key: "test"
key: "test"

and got this result:

YAMLException: duplicated mapping key at line 2, column 1:
    key: "test"
    ^

This error is also seen on the web version of the parser: https://nodeca.github.io/js-yaml/

[matteblair]

https://github.com/tangrams/tangram/blob/master/src/scene_bundle.js#L213-L217

We're currently using the 'json' option with js-yaml, which allows duplicate keys without throwing an exception. As noted in the comments here, Tangram ES currently allows duplicate keys too. Figuring out why Tangram ES behaves this way has reminded me of why we chose to leave this behavior as-is: Determining whether two keys are unique implies an equality function between them, and there is no such function in the YAML spec!

In most cases it's obvious whether two keys are equal.

11: first
11: second

But what about:

0b1011: first
0xB: second

Both keys are the integer 11, represented as different strings. In fact, js-yaml treats these keys as duplicates because they are equal after being cast to JS Numbers. In Tangram ES we can write some code to adjust the YAML parsing and get pretty granular about it. The challenging part would be figuring out how js-yaml / JS determines whether two scalars are equal and matching that behavior.

Tangram ES could implement a conservative approach that would cover the vast majority of cases: If two keys in a mapping are scalars, consider them duplicates if their string representations are equal.

[matkoniecz]

Tangram ES could implement a conservative approach

It would be enough for my usecase. I expect that in typical situation problem like this appears as line was duplicated, so key will be a string (is there a situation where in map style key will be not a string but something else?).

[matteblair]

is there a situation where in map style key will be not a string but something else?

Typically, no. Tangram only uses scalar keys in mappings, which means numbers, booleans, nulls, and strings are all allowed, but strings have always sufficed for us.