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

Fix geo_interface output when bbox is None #77

Merged

Conversation

yellowcap
Copy link
Member

@yellowcap yellowcap commented Jul 15, 2022

The bbox key should not be in a geojson object if the bbox is None. If the key is present, the value is expected to be a tuple, see https://datatracker.ietf.org/doc/html/rfc7946#section-5

In ths PR, I also moved the __geo_interface__ into a mixin to reduce code duplication.

There is one design question where I was not sure what is the best:
The bbox check could be added into the dict() method directly. Depending on whether the expected output of dict is a valid geojson like dictionary, or the "full" data from the pydantic model. Please advise, I am happy to change that if its considered better to put it into the dict method.

The bbox key should not be in a geojson object if its null
@geospatial-jeff
Copy link
Contributor

geospatial-jeff commented Jul 15, 2022

The bbox check could be added into the dict() method directly. Depending on whether the expected output of dict is a valid geojson like dictionary, or the "full" data from the pydantic model.

Personally I prefer it as written. Mutating the shape of the data on dict() may be unintuitive; but __geo_interface__ protocol is there for interoperability with other libraries (like shapely/geopandas etc.) and should follow the spec as close as possible even if that requires mutation.

The mixin works really nicely here too

@vincentsarago vincentsarago self-requested a review July 18, 2022 08:11
@vincentsarago vincentsarago merged commit c729cb8 into developmentseed:master Jul 18, 2022
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.

None yet

3 participants