Skip to content
This repository was archived by the owner on Aug 8, 2023. It is now read-only.

Conversation

@brunoabinader
Copy link
Member

Follow-up from #13450, turns out semantics was broken also for SymbolFeature. This PR replaces the optional usage in GeometryTileFeature::getValue(), converting non-existing property values to mapbox::feature::null_value_t when needed.

/cc @kkaefer @tmpsantos @asheemmamoowala for review.

@brunoabinader brunoabinader added the Core The cross-platform C++ core, aka mbgl label Nov 26, 2018
@brunoabinader brunoabinader self-assigned this Nov 26, 2018
@brunoabinader brunoabinader force-pushed the geometrytilefeature-getvalue-no-optional branch 2 times, most recently from 0b3b8bd to 194260f Compare November 26, 2018 15:38
@brunoabinader brunoabinader added the ⚠️ DO NOT MERGE Work in progress, proof of concept, or on hold label Nov 26, 2018
@brunoabinader brunoabinader force-pushed the geometrytilefeature-getvalue-no-optional branch from 194260f to fc264fb Compare November 30, 2018 12:32
@brunoabinader brunoabinader removed the ⚠️ DO NOT MERGE Work in progress, proof of concept, or on hold label Nov 30, 2018
@brunoabinader
Copy link
Member Author

@kkaefer @tmpsantos @asheemmamoowala please review. This clears the ambiguity with feature property values after removing optional usage from geometry.hpp interface.

Requires mapbox/vector-tile#53 to avoid copying the properties map whenever calling getProperties() in some cases.

FeatureType,
GeometryCollection,
std::unordered_map<std::string, std::string> properties = { {} });
PropertyMap properties = { {} });
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

&&?

return optional<Value>(it->second);
}
return optional<Value>();
return it != data->properties.cend() ? it->second : NullValue();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No need to use cend; The const overload of end will automatically be used since this function is const.

}
return optional<Value>();
Value getValue(const std::string& key) const override {
return feature.properties.count(key) ? feature.properties.at(key) : NullValue();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same

}
return optional<mbgl::Value>();
mbgl::Value getValue(const std::string& key) const override {
return feature.properties.count(key) ? feature.properties.at(key) : NullValue();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's not use this pattern; it does the same lookup twice, while the previous code only did it once.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My bad - forgot to update these.

@brunoabinader brunoabinader force-pushed the geometrytilefeature-getvalue-no-optional branch from fc264fb to f313863 Compare January 14, 2019 14:47
@stale stale bot added the archived Archived because of inactivity label Mar 15, 2019
@stale
Copy link

stale bot commented Mar 15, 2019

This pull request has been automatically detected as stale because it has not had recent activity and will be archived. Thank you for your contributions.

@stale stale bot closed this Mar 15, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

archived Archived because of inactivity Core The cross-platform C++ core, aka mbgl

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants