Simplify feature identifier method

Author: Conengmo

folium/features.py

@@ -25,7 +25,6 @@
     none_min,
     get_obj_in_upper_tree,
     parse_options,
-    dict_get,
 )
 from folium.vector_layers import PolyLine, path_options
 
@@ -530,21 +529,23 @@ def find_identifier(self):
            or even null.
 
         """
-        features = self.data['features']
-        n = len(features)
-        feature = features[0]
+        feats = self.data['features']
         # Each feature has an 'id' field with a unique value.
-        if 'id' in feature and len(set(feat['id'] for feat in features)) == n:
+        unique_ids = set(feat.get('id', None) for feat in feats)
+        if None not in unique_ids and len(unique_ids) == len(feats):
             return 'feature.id'
-        # Each feature has a unique string/float/int property.
-        #   'properties' may be missing or None:
-        if isinstance(feature.get('properties', None), dict):
-            field_name = self._search_properties(features, 'properties')
-            if field_name is not None:
-                return field_name
+        # Each feature has a unique string or int property.
+        if all(isinstance(feat.get('properties', None), dict) for feat in feats):
+            for key in feats[0]['properties']:
+                unique_values = set(
+                    feat['properties'].get(key, None) for feat in feats
+                    if isinstance(feat['properties'].get(key, None), (str, int))
+                )
+                if len(unique_values) == len(feats):
+                    return 'feature.properties.{}'.format(key)
         # We add an 'id' field with a unique value to the data.
         if self.embed:
-            for i, feature in enumerate(features):
+            for i, feature in enumerate(feats):
                 feature['id'] = str(i)
             return 'feature.id'
         raise ValueError(
@@ -553,30 +554,6 @@ def find_identifier(self):
             'field to your geojson data or set `embed=True`. '
         )
 
-    @classmethod
-    def _search_properties(cls, features, *keys):
-        """Find a property for which each feature has a unique str/num value."""
-        value_first_feature = dict_get(features[0], *keys)
-        # Recursively look through the dictionary to find a unique value.
-        if isinstance(value_first_feature, dict):
-            for key in value_first_feature:
-                field_name = cls._search_properties(features, *[*keys, key])
-                if field_name is not None:
-                    return field_name
-        # Check that all features have these keys and that the values are unique.
-        unique_values = set()
-        for feat in features:
-            try:
-                value = dict_get(feat, *keys)
-            except TypeError:
-                # not all features have the same properties layout
-                return None
-            if not isinstance(value, (str, float, int)):
-                return None
-            unique_values.add(value)
-        if len(unique_values) == len(features):
-            return '.'.join(['feature', *keys])
-
     def _get_self_bounds(self):
         """
         Computes the bounds of the object itself (not including it's children)

folium/utilities.py

@@ -490,20 +490,3 @@ def parse_options(**kwargs):
     return {camelize(key): value
             for key, value in kwargs.items()
             if value is not None}
-
-
-def dict_get(d, *keys):
-    """Return the field in dictionary d under the given keys.
-
-    Examples
-    --------
-    >>> dict_get({'hi': {'there': 42}}, 'hi', 'there')
-    42
-
-    """
-    if len(keys) == 0:
-        return d
-    if not isinstance(d, dict):
-        raise TypeError('The first argument should be a dictionary, not a {}.'
-                        .format(type(d)))
-    return dict_get(d[keys[0]], *keys[1:])

tests/test_features.py

@@ -215,70 +215,91 @@ def test_geojson_tooltip():
         assert issubclass(w[-1].category, UserWarning), 'GeoJsonTooltip GeometryCollection test failed.'
 
 
-def test_geojson_search_properties():
-    features = [{'properties': None} for _ in range(3)]
-    assert GeoJson._search_properties(features, 'properties') is None
-    features = [{'properties': {'hi': 'there'}},
-                {'properties': {'hi': 'what'}}]
-    assert GeoJson._search_properties(features, 'properties') == 'feature.properties.hi'
-    features = [{'properties': {'hi': {'more': 'some value'}}},
-                {'properties': {'hi': {'more': 'another value'}}}]
-    assert (GeoJson._search_properties(features, 'properties')
-            == 'feature.properties.hi.more')
-    features = [{'properties': {'hi': 'there'}},
-                {'properties': {'hi': 'there'}}]
-    assert GeoJson._search_properties(features, 'properties') is None
-    features = [{'properties': {'hi': 'there'}},
-                {'properties': {'hi': None}}]
-    assert GeoJson._search_properties(features, 'properties') is None
-    features = [{'properties': {'hi': 'there'}},
-                {'properties': 42}]
-    assert GeoJson._search_properties(features, 'properties') is None
-    features = [{'properties': [42, 43]},
-                {'properties': [1, 2]}]
-    assert GeoJson._search_properties(features, 'properties') is None
-
-
 def test_geojson_find_identifier():
 
-    def _create(properties):
+    def _create(*properties):
         return {"type": "FeatureCollection", "features": [
-            {"type": "Feature",
-             "properties": properties}
-        ]}
+            {"type": "Feature", "properties": item}
+            for item in properties]}
 
-    data_bare = _create(None)
-    data_with_id = _create(None)
+    def _assert_id_got_added(data):
+        _geojson = GeoJson(data)
+        assert _geojson.find_identifier() == 'feature.id'
+        assert _geojson.data['features'][0]['id'] == '0'
+
+    data_with_id = _create(None, None)
     data_with_id['features'][0]['id'] = 'this-is-an-id'
-    data_with_unique_property = _create({
-        'property-key': 'some-value',
-    })
+    data_with_id['features'][1]['id'] = 'this-is-another-id'
+    geojson = GeoJson(data_with_id)
+    assert geojson.find_identifier() == 'feature.id'
+    assert geojson.data['features'][0]['id'] == 'this-is-an-id'
+
+    data_with_unique_properties = _create(
+        {'property-key': 'some-value'},
+        {'property-key': 'another-value'},
+    )
+    geojson = GeoJson(data_with_unique_properties)
+    assert geojson.find_identifier() == 'feature.properties.property-key'
+
+    data_with_unique_properties = _create(
+        {'property-key': 42},
+        {'property-key': 43},
+        {'property-key': 'or a string'},
+    )
+    geojson = GeoJson(data_with_unique_properties)
+    assert geojson.find_identifier() == 'feature.properties.property-key'
+
+    # The test cases below have no id field or unique property,
+    # so an id will be added to the data.
+
+    data_with_identical_ids = _create(None, None)
+    data_with_identical_ids['features'][0]['id'] = 'identical-ids'
+    data_with_identical_ids['features'][1]['id'] = 'identical-ids'
+    _assert_id_got_added(data_with_identical_ids)
+
+    data_with_some_missing_ids = _create(None, None)
+    data_with_some_missing_ids['features'][0]['id'] = 'this-is-an-id'
+    # the second feature doesn't have an id
+    _assert_id_got_added(data_with_some_missing_ids)
+
+    data_with_identical_properties = _create(
+        {'property-key': 'identical-value'},
+        {'property-key': 'identical-value'},
+    )
+    _assert_id_got_added(data_with_identical_properties)
+
+    data_bare = _create(None)
+    _assert_id_got_added(data_bare)
+
+    data_empty_dict = _create({})
+    _assert_id_got_added(data_empty_dict)
+
+    data_without_properties = _create(None)
+    del data_without_properties['features'][0]['properties']
+    _assert_id_got_added(data_without_properties)
+
+    data_some_without_properties = _create({'key': 'value'}, 'will be deleted')
+    # the first feature has properties, but the second doesn't
+    del data_some_without_properties['features'][1]['properties']
+    _assert_id_got_added(data_some_without_properties)
+
     data_with_nested_properties = _create({
         "summary": {"distance": 343.2},
         "way_points": [3, 5],
     })
+    _assert_id_got_added(data_with_nested_properties)
+
     data_with_incompatible_properties = _create({
         "summary": {"distances": [0, 6], "durations": None},
         "way_points": [3, 5],
     })
-
-    geojson = GeoJson(data_with_id)
-    assert geojson.find_identifier() == 'feature.id'
-    geojson = GeoJson(data_bare)
-    assert geojson.find_identifier() == 'feature.id'
-    assert geojson.data['features'][0]['id'] == '0'  # the id got added
-    geojson = GeoJson(data_with_unique_property)
-    assert geojson.find_identifier() == 'feature.properties.property-key'
-    geojson = GeoJson(data_with_nested_properties)
-    assert geojson.find_identifier() == 'feature.properties.summary.distance'
-    geojson = GeoJson(data_with_incompatible_properties)
-    assert geojson.find_identifier() == 'feature.id'
-    assert geojson.data['features'][0]['id'] == '0'  # the id got added
+    _assert_id_got_added(data_with_incompatible_properties)
 
     data_loose_geometry = {"type": "LineString", "coordinates": [
         [3.961389, 43.583333], [3.968056, 43.580833], [3.974722, 43.578333],
         [3.986389, 43.575278], [3.998333, 43.5725], [4.163333, 43.530556],
     ]}
     geojson = GeoJson(data_loose_geometry)
     geojson.convert_to_feature_collection()
-    assert geojson.find_identifier() == 'feature.id'  # id got added
+    assert geojson.find_identifier() == 'feature.id'
+    assert geojson.data['features'][0]['id'] == '0'

tests/test_utilities.py

@@ -11,7 +11,6 @@
     deep_copy,
     get_obj_in_upper_tree,
     parse_options,
-    dict_get,
 )
 
 
@@ -155,16 +154,3 @@ def test_parse_options():
     assert parse_options(thing=None) == {}
     assert parse_options(long_thing=42) == {'longThing': 42}
     assert parse_options(thing=42, lst=[1, 2]) == {'thing': 42, 'lst': [1, 2]}
-
-
-def test_dict_get():
-    assert dict_get({}) == {}
-    assert dict_get({'hi': 'there'}) == {'hi': 'there'}
-    assert dict_get({'hi': {'there': 42}}, 'hi', 'there') == 42
-    assert dict_get({'hi': [1, 2, 3]}, 'hi') == [1, 2, 3]
-    with pytest.raises(TypeError):
-        dict_get({'hi': 42}, 'hi', 'wrong-key')
-    with pytest.raises(TypeError):
-        dict_get(42, 'wrong-key')
-    with pytest.raises(KeyError):
-        dict_get({'hi': 42}, 'wrong-key')