Localize all or nothing #129

lily-chai · 2022-10-25T21:02:20Z

This PR cleans up the function localize. This function now only returns either

features with all language, worldview, class properties localized; or
features with none of the language, worldview, class properties localized.

See README.md for more detail.

… class

…iews

This reverts commit 05dc619.

CONTRIBUTING.md

johnk1

Haven't gotten too far yet, but wanted to send what I have before a meeting

README.md

package.json

src/module_utils.hpp

johnk1 · 2022-10-26T17:02:01Z

src/vtcomposite.cpp

-            // no-op on any property called "worldview" if worldviews have been
-            // created dynamically from _mbx_wordlview
-            if (!worldview.empty() && property.first == "worldview")
+            if (property.first == worldview_key) // safeguard – should always evaluate to false


why should this always be false?

Because in the current implementation, when we encounter {worldview_property} or {worldview_prefix}{worldview_property} while looping through the properties, we only store the worldviews to create in the worldviews_to_create variable but not add them to the final_properties vector. After we're done looping through the properties, we decide how many clones of the feature to build (for now we are limiting to 1) and what each clone should have as its {worldview_property} .

This safeguard is to prevent future code change that might accidentally/unintentionally add worldview to final_properties, and also to make clear for humans that worldview_key of the property always come from worldview_val.

src/vtcomposite.cpp

johnk1 · 2022-10-26T17:10:38Z

src/vtcomposite.cpp

                    while (auto property = feature.next_property())
                    {
+                        // if already know we'll be skipping this feature, don't need to comb through its properties
+                        if (skip_feature)


could this added to the while? while (property = next_property && !skip_feature)

Unfortunately no, that turns property into bool type.

To be clear I meant something like
while (!skip_feature && auto feature = layer.next_feature())
rather than have the skip_feature check on L739.
It's not a very important detail but would be a little more readable IMO

The while loop while (auto feature = layer.next_feature()) is for looping through the features one by one; we don't know whether we can skip a feature or not before we look at its properties, so skip_feature is always false when we evaluate this while condition. The skip_feature flag changes to true when we encounter a property that suggests we can discard this feature, and the flag resets to false when we are done with one feature and moves on to the next feature. I'll add a comment to clarify this.

while (!skip_feature && auto property = feature.next_property()) results in this error:

../src/vtcomposite.cpp:736:45: error: expected expression while (!skip_feature && auto property = feature.next_property()) ^ ../src/vtcomposite.cpp:744:52: error: use of undeclared identifier 'property' std::string property_key = property.key().to_string(); ^

and while (auto property = feature.next_property() && !skip_feature) results in this error:

../src/vtcomposite.cpp:744:60: error: member reference base type 'bool' is not a structure or union std::string property_key = property.key().to_string(); ~~~~~~~~^~~~

Added more details to the comments re: skip_feature in 759e709.

src/vtcomposite.cpp

johnk1

This looks good to me! My only real request is to move to v2. Other things are less critical code style and so on.

mapsam

@lily-chai initial read-through looks really great, nice to see some explicit variable names set to help dictate the flow of the feature/property parsing.

I left some questions just to ask for clarifications. But after reading through all of your tests I'm feeling quite confident in the changes here!

package.json

mapsam · 2022-10-26T17:59:26Z

src/vtcomposite.cpp

-                language_key = baton_data_->language_property + "_" + baton_data_->language;
-                // {language_prefix}{language_property}_{language} -> drop
-                language_key_prefixed = baton_data_->language_prefix + language_key;
+                keep_every_worldview = true;


why reset to true if the default is already true?

Just for clarity – this way it's easier for the human brain to see the difference between the configuration localized tile (the if block) and the configuration for non-localized tile (the else block). If you're not against leaving this in, I'll add a comment to explain this.

Added a comment in 3711e05.

src/vtcomposite.cpp

test/vtcomposite-localize-class.test.js

mapsam · 2022-10-26T18:16:58Z

test/vtcomposite-localize-class.test.js

+  localize(params, (err, vtBuffer) => {
+    assert.ifError(err);
+    const tile = vtinfo(vtBuffer);
+    assert.deepEqual(tile.layers, {}, 'has no feature');


just to confirm, we're dropping this feature because we anticipate another feature with _mbx_worldview to exist with the value assigned as US ?

mapsam · 2022-10-26T18:21:11Z

test/vtcomposite-localize-language.test.js

-    'name_fr': 'Allemagne', // choosing first encountered property in (name_fr, _mbx_name_fr)
+    name: 'Allemagne',  // name_{language} takes precedence over _mbx_name_{language}
+    name_local: 'Germany',
+    _mbx_other: 'Alemania'


why don't we drop arbitrary _mbx* fields anymore?

Ask because I'd expect someone to add fields to their data expecting us to remove them as "internal-only" fields.

I made the params for language, worldview and class more consistent:

Before: we can specify a prefix for language, but not for class and worldview. It isn't clear why.

After: we can specify a prefix for language, worldview and class. The prefixes can be different.

But that makes dropping fields confusing: for example, do we drop fields that match any of the 3 prefixes? Why do we want to drop a field that is irrelevant to e.g. worldview only because it has the the same prefix as worldview?

If we want to drop arbitrary _mbx_* fields, I suggest having just one prefix param and call it something like params.hidden_prefix.

If we want to drop arbitrary mbx* fields, I suggest having just one prefix param and call it something like params.hidden_prefix.

Irrespective of dropping all fields or not - I like this idea just to simplify the interface. I don't guess someone would set a different prefix for each kind of localizable field, yea?

Changed to one hidden_prefix and drops any property that starts with this prefix in 00b9398.

Co-authored-by: Sam Matthews <sam@mapbox.com>

Co-authored-by: John Klancer <john.klancer@mapbox.com>

johnk1

🎈 thanks for taking on this slightly head spinning work!

lily-chai added 30 commits October 17, 2022 12:50

add note in contributing.md

9555ca9

[revert me] comment out make_global_settings

05dc619

make LocalizeBatonType members consistent

c23a5f9

validate new LocalizeBatonType members in localize function

2d789e3

add return_localized_tile to LocalizeBatonType

06725fa

retain only those features with compatible worldview and adjust their…

617d4d5

… class

be more explicity with class

e61a624

easier to read

2e89c7e

consistent bracket format

61e591e

add string startswith helper function

c70ddb8

not all layers have both {class_prefix}{class_property}

597d561

non-localized tile need to keep features in every worldview

fe595bd

keep only relevant names

8eb553e

do not interchange name and language

fe48ec0

fix typos and syntax errors

0f28295

fix class test

2b1f149

fix worldview test

dd809d2

add worldview_default; enfore non-empty array of languages and worldv…

8d62e75

…iews

test default worldview

a5b94bb

test invalid params.worldviews values

ff88492

lint

3bd5f32

split up vtcomposite-param-validation.test.js by function

84f5baa

fix localize param validation tests

c8750a2

fix default worldview code, add partial string match test

938ef20

fix language test

7825b6b

add checks for legacy params

287fc20

Revert "[revert me] comment out make_global_settings"

32665e7

This reverts commit 05dc619.

update readme

ae37679

fix format

30eac5f

tidy up

fd5479e

lily-chai added 2 commits October 25, 2022 13:54

preserve ability to create clones of a feature

3aa513e

increment version, update changelog

06255f4

lily-chai force-pushed the localised-vs-nonlocalised branch from cda60d6 to 06255f4 Compare October 25, 2022 21:11

1.2.0-dev0 [publish binary]

4931c25

lily-chai marked this pull request as ready for review October 25, 2022 21:16

lily-chai requested a review from a team October 25, 2022 21:16

johnk1 reviewed Oct 26, 2022

View reviewed changes

CONTRIBUTING.md Show resolved Hide resolved

johnk1 reviewed Oct 26, 2022

View reviewed changes

README.md Outdated Show resolved Hide resolved

README.md Outdated Show resolved Hide resolved

README.md Outdated Show resolved Hide resolved

package.json Outdated Show resolved Hide resolved

src/module_utils.hpp Outdated Show resolved Hide resolved

johnk1 reviewed Oct 26, 2022

View reviewed changes

johnk1 requested changes Oct 26, 2022

View reviewed changes

mapsam approved these changes Oct 26, 2022

View reviewed changes

lily-chai and others added 3 commits October 27, 2022 05:12

fix comment

cf0cee6

Co-authored-by: Sam Matthews <sam@mapbox.com>

fix README.md

328092e

Co-authored-by: John Klancer <john.klancer@mapbox.com>

address feedback

3711e05

lily-chai force-pushed the localised-vs-nonlocalised branch from a32bea9 to df9b1f4 Compare October 26, 2022 23:52

remove stray &&

f7236b3

lily-chai force-pushed the localised-vs-nonlocalised branch from df9b1f4 to f7236b3 Compare October 27, 2022 00:02

lily-chai requested a review from johnk1 October 27, 2022 00:07

johnk1 approved these changes Oct 27, 2022

View reviewed changes

lily-chai added 4 commits October 27, 2022 09:12

update changelog

47f5e10

elaborate comment

759e709

one hidden_prefix for all; drop anything with hidden_prefix

00b9398

2.0.0 [publish binary]

bd7647f

lily-chai merged commit d01d84d into main Oct 27, 2022

lily-chai deleted the localised-vs-nonlocalised branch October 27, 2022 18:03

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Localize all or nothing #129

Localize all or nothing #129

lily-chai commented Oct 25, 2022

johnk1 left a comment

johnk1 Oct 26, 2022 •

edited

Loading

lily-chai Oct 26, 2022

johnk1 Oct 26, 2022

lily-chai Oct 26, 2022

johnk1 Oct 27, 2022

lily-chai Oct 27, 2022

lily-chai Oct 27, 2022

johnk1 left a comment

mapsam left a comment

mapsam Oct 26, 2022

lily-chai Oct 26, 2022

lily-chai Oct 27, 2022

mapsam Oct 26, 2022

lily-chai Oct 26, 2022

mapsam Oct 26, 2022

lily-chai Oct 26, 2022

johnk1 Oct 27, 2022

lily-chai Oct 27, 2022

johnk1 left a comment

Localize all or nothing #129

Localize all or nothing #129

Conversation

lily-chai commented Oct 25, 2022

johnk1 left a comment

Choose a reason for hiding this comment

johnk1 Oct 26, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

johnk1 left a comment

Choose a reason for hiding this comment

mapsam left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

johnk1 left a comment

Choose a reason for hiding this comment

johnk1 Oct 26, 2022 •

edited

Loading