Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add Feature, Geometry and ObjectEncoder classes to model #787

Merged
merged 25 commits into from
Apr 26, 2022
Merged

Conversation

sgillies
Copy link
Member

@sgillies sgillies commented Sep 19, 2019

Resolves #760
Resolves #758

@coveralls
Copy link

coveralls commented Sep 19, 2019

Coverage Status

Coverage increased (+5.1%) to 87.824% when pulling 9ce6f34 on issue760 into 24755a2 on maint-1.9.

Fiona yields properties=Properties instead of OrderedDict.
@@ -5,7 +5,7 @@ from __future__ import absolute_import
import logging

from fiona.errors import UnsupportedGeometryTypeError

from fiona.model import GEOMETRY_TYPES, Geometry
Copy link
Member Author

Choose a reason for hiding this comment

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

To avoid circular import.

@@ -127,6 +128,9 @@ def _transform_geom(
cdef void *dst_ogr_geom = NULL
cdef int i

if not isinstance(geom, Geometry):
geom = Geometry.from_dict(**geom)

Copy link
Member Author

Choose a reason for hiding this comment

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

This is perhaps better done in transform.py. We could strictly require Geometry in the extension module since it isn't in the public API.

@sgillies
Copy link
Member Author

@jorisvandenbossche you might want to have a look at this too. Fiona 1.9 will still accept data as dicts but will start returning dict-like Feature objects that are not instances of dict.

I'm eager to get this merged in so I can move on to supporting GDAL 3.0.

@jorisvandenbossche
Copy link
Member

@sgillies this looks good to me. I quickly tried out this branch with geopandas, and as expected all tests pass without problems.

fiona/model.py Outdated Show resolved Hide resolved
tests/test_model.py Outdated Show resolved Hide resolved
tests/test_model.py Outdated Show resolved Hide resolved
Copy link
Member

@snorfalorpagus snorfalorpagus left a comment

Choose a reason for hiding this comment

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

Looks good. I've requested some minor changes. My main question is why the need for Geometry/_Geometry?

@sgillies
Copy link
Member Author

@snorfalorpagus to your question, very belatedly 😆 , the Geometry/_Geometry design was is anticipation of rewriting _Geometry as a Cython extension class, but now I think it was misguided. I'll probably undo it in a future PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants