Deprecate mutability of Point, LineString, LinearRing geometries#900
Conversation
|
Any feedback on this PR? |
tests/test_point.py
Outdated
| import pytest | ||
|
|
||
|
|
||
| shapely20_deprecated = pytest.mark.filterwarnings("ignore:Setting:FutureWarning") |
There was a problem hiding this comment.
Having written a couple of these markers for specifying required GEOS versions, I was wondering if we should put them in a common place (like conftest.py) so they don't need to be re-declared in every file in which they're used. Especially for one like this, which will likely get re-used many times in the coming months.
There was a problem hiding this comment.
It was lost on me at the time of commenting that the :Setting: part in the middle is referencing the text of the deprecation warning, and won't work for other messages that don't start with Setting. Should there be a standard message that can be used everywhere deprecations are occurring?
There was a problem hiding this comment.
Should there be a standard message that can be used everywhere deprecations are occurring?
Yes, that sounds a good idea to have all messages that will be related to "Shapely 2.0 deprecations" have a consistent structure. That makes filtering them out like the above easier with a single pytest marker, and is also more recognizable for users.
(we could actually also go for a warning subclass, for example sphinx has "RemovedInSphinx4Warning": https://github.com/sphinx-doc/sphinx/blob/776ec7957c3407a5ed75dc4369a0042d91698bd1/sphinx/deprecation.py#L21, but that can also easily be adopted later if we want to do so)
I was wondering if we should put them in a common place (like conftest.py) so they don't need to be re-declared in every file in which they're used.
Yes, certainly! It was just because I was only editing a single file here, but once I do the same for the other geometry classes, we certainly need to move it to a general place (and conftest.py seems appropriate for that with pytest)
There was a problem hiding this comment.
I quite like the idea of a RemovedInShapely20 warning subclass. Would make it much easier for users to filter out all of those warnings with a single go, if they'd like to.
There was a problem hiding this comment.
I should have scrolled down here before I recommended the new class above :)
|
@sgillies are you OK with the approach here? If so, I can update this PR to cover all geometry types. (although RFC 1 is not yet officially approved, I hope I can start on some of the non-controversial parts, like deprecating mutability) |
shapely/geometry/point.py
Outdated
| warnings.warn( | ||
| "Setting the 'coords' to mutate a Geometry in place is deprecated," | ||
| " and will not be possible any more in Shapely 2.0", | ||
| FutureWarning, stacklevel=2) |
There was a problem hiding this comment.
@jorisvandenbossche what would you think about adding a new ShapelyDeprecationWarning as recommended in
https://www.python.org/dev/peps/pep-0565/#additional-use-case-for-futurewarning ?
For library and framework authors that want to ensure their API compatibility warnings are more reliably seen by their users, the recommendation is to use a custom warning class that derives from DeprecationWarning in Python 3.7+, and from FutureWarning in earlier versions.
|
@jorisvandenbossche I'm in favor of this 👍 Let's do consider a new shapely warning class. |
|
I added a |
|
Also a workflow question: do I continue here in this PR with doing the same for the other geometry types, or do you prefer to keep that for a separate PR(s)? |
|
@jorisvandenbossche it's all the same to me, one or multiple PRs as you prefer. |
|
OK, I added deprecation warnings for linestrings and linearrings as well (I think that's all, since collections' coords are not directly mutable (only their parts), and the same for polygons and its rings) So this should be ready for review / merge |
| """ | ||
| Warning for features that will be removed or behaviour that will be | ||
| changed in a future release. | ||
| """ |
| self._set_coords(coordinates) | ||
| ret = geos_linestring_from_py(coordinates) | ||
| if ret is not None: | ||
| self._geom, self._ndim = ret |
There was a problem hiding this comment.
@jorisvandenbossche I appreciate how we will be able to just delete _set_coords in the future. Thank you!
It seems you deleted this comment (or I don't find it back in the github UI), but yes, we could do that, to ensure that we are explicit in the tests about what is deprecated (and don't miss a consequence elsewhere in the package). |
|
And thanks for merging! |
|
Excellent! I'm really excited about getting a 1.8.0 out. I've got time this afternoon to review tickets and do some release planning. The sooner, the better. Maybe we can even skip a 1.7.1. |
xref #782 (Shapely 2.0).
Example of deprecating mutability this for Point:
coordssetterIf that workflow looks OK, I can do the same for the other geometry types.