Repository: Toblerity/Shapely
Pull request #950 (2020-07-28 01:52:50)

Add deprecation warnings for iteration & getitem in multipart geometries
As per #932, this adds deprecation warnings for `__iter__` and `__getitem__` in multi-part geometries.

Looks like `stacklevel=1` gives a message like:

```
/Users/tom/dev/python/contrib/Shapely/shapely/geometry/base.py:880: ShapelyDeprecationWarning: Iteration over multi-part geometries is deprecated and will be removed in Shapely 2.0. Use the `geoms` property to access the constituent parts of a multi-part geometry.
```

which feels right to me.

coveralls: 
[![Coverage Status](https://coveralls.io/builds/32359484/badge)](https://coveralls.io/builds/32359484)

Coverage increased (+0.01%) to 82.106% when pulling **c0ef04e84fddcaaf4604c415a7a00065b614a333 on tomplex:deprecate-iteration** into **24a9bf768315d7293bb31cc46d1f22a84cb8f7ea on Toblerity:master**.

tomplex: Looking into #952 showed me a couple places where the library itself actually depends on length / iteration over geometries, so there's more to do here than I realized.
tomplex: Do we want to deprecate `__len__` as well? A `len` usually implies iterability, and if we're telling users to use the `geoms` property to access individual parts and to iterate, it feels like they should have to use `geoms` to get a `len`, as well.
jorisvandenbossche: > do we want to deprecate `__len__` as well?

Ah, that's a good point, and something we didn't consider yet in the RFC. 

I agree that kind of follows out of deprecating iteration/getitem, and similarly users can switch `len(obj)` to `len(obj.geoms)`. 
An alternative could be to add a "length" to *all* types of geometries, where the single-part geometries like Point, LineString, Polygon would have a length of 1. 

(for this PR we can maybe focus on iteration/getitem, since that seems already quite some work anyways?)
tomplex: Sure, we address deprecation (or other changes to) `__len__` for a later PR. I'll add something to the RFC later today to try to get some discussion going.
tomplex: Hey @jorisvandenbossche, despite my best efforts I've been struggling to find time to work on this, so if you want to add to it, please feel free.
jorisvandenbossche: @tomplex I updated this: merged latest master + fixed some remaining usages of getitem in the tests. The full test build now runs without showing any warning. 

This should be good for a final review now.

Patch:
diff --git a/docs/manual.rst b/docs/manual.rst
index 895debe..68a94f0 100644
--- a/docs/manual.rst
+++ b/docs/manual.rst
@@ -392,7 +392,7 @@ copy.
   >>> LineString(line)
   <shapely.geometry.linestring.LineString object at 0x...>
 
-A `LineString` may also be constructed using a a sequence of mixed `Point`
+A `LineString` may also be constructed using a sequence of mixed `Point`
 instances or coordinate tuples. The individual coordinates are copied into
 the new object.
 
diff --git a/shapely/geometry/base.py b/shapely/geometry/base.py
index 4e0e76e..238853e 100644
--- a/shapely/geometry/base.py
+++ b/shapely/geometry/base.py
@@ -846,6 +846,16 @@ class BaseMultipartGeometry(BaseGeometry):
         return self.is_empty is False
 
     def __iter__(self):
+        """
+        .. deprecated:: 1.8
+           Iteration over multi-part geometries is deprecated and will be removed in
+           Shapely 2.0. Use the `geoms` property to access the constituent parts of
+           a multi-part geometry.
+        """
+        warn(
+            "Iteration over multi-part geometries is deprecated and will be removed in "
+            "Shapely 2.0. Use the `geoms` property to access the constituent parts of " 
+            "a multi-part geometry.", ShapelyDeprecationWarning, stacklevel=2)
         if not self.is_empty:
             return iter(self.geoms)
         else:
@@ -858,6 +868,16 @@ class BaseMultipartGeometry(BaseGeometry):
             return 0
 
     def __getitem__(self, index):
+        """
+        .. deprecated:: 1.8
+           __getitem__ for multi-part geometries is deprecated and will be removed in
+           Shapely 2.0. Use the `geoms` property to access the constituent parts of
+           a multi-part geometry.
+        """
+        warn(
+            "__getitem__ for multi-part geometries is deprecated and will be removed in "
+            "Shapely 2.0. Use the `geoms` property to access the constituent parts of " 
+            "a multi-part geometry.", ShapelyDeprecationWarning, stacklevel=2)
         if not self.is_empty:
             return self.geoms[index]
         else:
diff --git a/shapely/geometry/collection.py b/shapely/geometry/collection.py
index 3c9d333..eccd6bf 100644
--- a/shapely/geometry/collection.py
+++ b/shapely/geometry/collection.py
@@ -56,6 +56,8 @@ class GeometryCollection(BaseMultipartGeometry):
 
 def geos_geometrycollection_from_py(ob):
     """Creates a GEOS GeometryCollection from a list of geometries"""
+    if isinstance(ob, BaseMultipartGeometry):
+         ob = ob.geoms
     L = len(ob)
     N = 2
     subs = (c_void_p * L)()


Test patch:
diff --git a/tests/test_getitem.py b/tests/test_getitem.py
index cd31770..1024132 100644
--- a/tests/test_getitem.py
+++ b/tests/test_getitem.py
@@ -1,4 +1,4 @@
-from . import unittest
+from . import unittest, shapely20_deprecated
 from shapely import geometry
 
 
@@ -48,6 +48,7 @@ class CoordsGetItemTestCase(unittest.TestCase):
 
 class MultiGeomGetItemTestCase(unittest.TestCase):
 
+    @shapely20_deprecated
     def test_index_multigeom(self):
         c = [(float(x), float(-x)) for x in range(4)]
         g = geometry.MultiPoint(c)
@@ -56,11 +57,13 @@ class MultiGeomGetItemTestCase(unittest.TestCase):
         self.assertRaises(IndexError, lambda: g[4])
         self.assertRaises(IndexError, lambda: g[-5])
 
+    @shapely20_deprecated
     def test_index_multigeom_misc(self):
         g = geometry.MultiLineString()  # empty
         self.assertRaises(IndexError, lambda: g[0])
         self.assertRaises(TypeError, lambda: g[0.0])
 
+    @shapely20_deprecated
     def test_slice_multigeom(self):
         c = [(float(x), float(-x)) for x in range(4)]
         g = geometry.MultiPoint(c)
diff --git a/tests/test_multilinestring.py b/tests/test_multilinestring.py
index 8cc3971..6513bfa 100644
--- a/tests/test_multilinestring.py
+++ b/tests/test_multilinestring.py
@@ -32,8 +32,8 @@ class MultiLineStringTestCase(MultiGeometryTestCase):
 
         # Sub-geometry Access
         geom = MultiLineString([(((0.0, 0.0), (1.0, 2.0)))])
-        self.assertIsInstance(geom[0], LineString)
-        self.assertEqual(dump_coords(geom[0]), [(0.0, 0.0), (1.0, 2.0)])
+        self.assertIsInstance(geom.geoms[0], LineString)
+        self.assertEqual(dump_coords(geom.geoms[0]), [(0.0, 0.0), (1.0, 2.0)])
         with self.assertRaises(IndexError):  # index out of range
             geom.geoms[1]
 
@@ -42,7 +42,6 @@ class MultiLineStringTestCase(MultiGeometryTestCase):
                          {'type': 'MultiLineString',
                           'coordinates': (((0.0, 0.0), (1.0, 2.0)),)})
 
-
     def test_from_multilinestring_z(self):
         coords1 = [(0.0, 1.0, 2.0), (3.0, 4.0, 5.0)]
         coords2 = [(6.0, 7.0, 8.0), (9.0, 10.0, 11.0)]
@@ -85,6 +84,7 @@ class MultiLineStringTestCase(MultiGeometryTestCase):
 
         # TODO: is there an inverse?
 
+    @shapely20_deprecated
     def test_subgeom_access(self):
         line0 = LineString([(0.0, 1.0), (2.0, 3.0)])
         line1 = LineString([(4.0, 5.0), (6.0, 7.0)])
@@ -106,5 +106,18 @@ def test_multilinestring_adapter_deprecated():
         asMultiLineString(coords)
 
 
+def test_iteration_deprecated():
+    geom = MultiLineString([[[5.0, 6.0], [7.0, 8.0]]])
+    with pytest.warns(ShapelyDeprecationWarning, match="Iteration"):
+        for g in geom:
+            pass
+
+
+def test_getitem_deprecated():
+    geom = MultiLineString([[[5.0, 6.0], [7.0, 8.0]]])
+    with pytest.warns(ShapelyDeprecationWarning, match="__getitem__"):
+        part = geom[0]
+
+
 def test_suite():
     return unittest.TestLoader().loadTestsFromTestCase(MultiLineStringTestCase)
diff --git a/tests/test_multipoint.py b/tests/test_multipoint.py
index 55f4e9e..5fed9a4 100644
--- a/tests/test_multipoint.py
+++ b/tests/test_multipoint.py
@@ -89,6 +89,7 @@ class MultiPointTestCase(MultiGeometryTestCase):
         pas = asarray(geoma)
         assert_array_equal(pas, array([[1., 2.], [3., 4.]]))
 
+    @shapely20_deprecated
     def test_subgeom_access(self):
         p0 = Point(1.0, 2.0)
         p1 = Point(3.0, 4.0)
@@ -128,5 +129,18 @@ def test_multipoint_array_interface_numpy_deprecated():
         np.array(geom)
 
 
+def test_iteration_deprecated():
+    geom = MultiPoint([[5.0, 6.0], [7.0, 8.0]])
+    with pytest.warns(ShapelyDeprecationWarning, match="Iteration"):
+        for g in geom:
+            pass
+
+
+def test_getitem_deprecated():
+    geom = MultiPoint([[5.0, 6.0], [7.0, 8.0]])
+    with pytest.warns(ShapelyDeprecationWarning, match="__getitem__"):
+        part = geom[0]
+
+
 def test_suite():
     return unittest.TestLoader().loadTestsFromTestCase(MultiPointTestCase)
diff --git a/tests/test_multipolygon.py b/tests/test_multipolygon.py
index d1c58dc..143e4a4 100644
--- a/tests/test_multipolygon.py
+++ b/tests/test_multipolygon.py
@@ -74,6 +74,7 @@ class MultiPolygonTestCase(MultiGeometryTestCase):
         self.assertEqual(len(mpa.geoms[0].interiors), 1)
         self.assertEqual(len(mpa.geoms[0].interiors[0].coords), 5)
 
+    @shapely20_deprecated
     def test_subgeom_access(self):
         poly0 = Polygon([(0.0, 0.0), (0.0, 1.0), (1.0, 1.0), (1.0, 0.0)])
         poly1 = Polygon([(0.25, 0.25), (0.25, 0.5), (0.5, 0.5), (0.5, 0.25)])
@@ -96,3 +97,20 @@ def test_multipolygon_adapter_deprecated():
     holes_coords = [((0.25, 0.25), (0.25, 0.5), (0.5, 0.5), (0.5, 0.25))]
     with pytest.warns(ShapelyDeprecationWarning, match="proxy geometries"):
         asMultiPolygon([(coords, holes_coords)])
+
+
+def test_iteration_deprecated():
+    geom = MultiPolygon(
+        [(((0.0, 0.0), (0.0, 1.0), (1.0, 1.0), (1.0, 0.0)),
+          [((0.25, 0.25), (0.25, 0.5), (0.5, 0.5), (0.5, 0.25))])])
+    with pytest.warns(ShapelyDeprecationWarning, match="Iteration"):
+        for g in geom:
+            pass
+
+
+def test_getitem_deprecated():
+    geom = MultiPolygon(
+        [(((0.0, 0.0), (0.0, 1.0), (1.0, 1.0), (1.0, 0.0)),
+          [((0.25, 0.25), (0.25, 0.5), (0.5, 0.5), (0.5, 0.25))])])
+    with pytest.warns(ShapelyDeprecationWarning, match="__getitem__"):
+        part = geom[0]
diff --git a/tests/test_shared_paths.py b/tests/test_shared_paths.py
index bf2db1b..24d6f51 100644
--- a/tests/test_shared_paths.py
+++ b/tests/test_shared_paths.py
@@ -17,10 +17,10 @@ class SharedPaths(unittest.TestCase):
         a, b = result.geoms
         self.assertTrue(isinstance(a, MultiLineString))
         self.assertTrue(len(a) == 1)
-        self.assertEqual(a[0].coords[:], [(5, 0), (10, 0)])
+        self.assertEqual(a.geoms[0].coords[:], [(5, 0), (10, 0)])
         self.assertTrue(b.is_empty)
 
-    def test_shared_paths_forward(self):
+    def test_shared_paths_forward2(self):
         g1 = LineString([(0, 0), (10, 0), (10, 5), (20, 5)])
         g2 = LineString([(15, 0), (5, 0)])
         result = shared_paths(g1, g2)
@@ -30,7 +30,7 @@ class SharedPaths(unittest.TestCase):
         a, b = result.geoms
         self.assertTrue(isinstance(b, MultiLineString))
         self.assertTrue(len(b) == 1)
-        self.assertEqual(b[0].coords[:], [(5, 0), (10, 0)])
+        self.assertEqual(b.geoms[0].coords[:], [(5, 0), (10, 0)])
         self.assertTrue(a.is_empty)
     
     def test_wrong_type(self):
diff --git a/tests/test_split.py b/tests/test_split.py
index 9e3a253..58f759a 100644
--- a/tests/test_split.py
+++ b/tests/test_split.py
@@ -23,7 +23,7 @@ class TestSplitGeometry(unittest.TestCase):
 				raise ValueError
 		elif expected_chunks == 1:
 			# not split --> expected equal to line
-			self.assertTrue(s[0].equals(geom))
+			self.assertTrue(s.geoms[0].equals(geom))
 
 	def test_split_closed_line_with_point(self):
 		# point at start/end of closed ring -> return equal
@@ -175,15 +175,15 @@ class TestSplitClosedRing(TestSplitGeometry):
 		splitter = Point([0.0, 0.5])
 		self.helper(self.ls, splitter, 2)
 		result = split(self.ls, splitter)
-		assert result[0].coords[:] == [(0, 0), (0.0, 0.5)]
-		assert result[1].coords[:] == [(0.0, 0.5), (0, 1), (1, 1), (1, 0), (0, 0)]
+		assert result.geoms[0].coords[:] == [(0, 0), (0.0, 0.5)]
+		assert result.geoms[1].coords[:] == [(0.0, 0.5), (0, 1), (1, 1), (1, 0), (0, 0)]
 
 		# previously failed, see GH#585
 		splitter = Point([0.5, 0.0])
 		self.helper(self.ls, splitter, 2)
 		result = split(self.ls, splitter)
-		assert result[0].coords[:] == [(0, 0), (0, 1), (1, 1), (1, 0), (0.5, 0)]
-		assert result[1].coords[:] == [(0.5, 0), (0, 0)]
+		assert result.geoms[0].coords[:] == [(0, 0), (0, 1), (1, 1), (1, 0), (0.5, 0)]
+		assert result.geoms[1].coords[:] == [(0.5, 0), (0, 0)]
 
 		splitter = Point([2.0, 2.0])
 		self.helper(self.ls, splitter, 1)