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.

Sign up for GitHub

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

Jump to bottom

Allow None to be passed for geoms and bboxes on features #56

Merged
merged 6 commits into from
Apr 28, 2022
Merged

Allow None to be passed for geoms and bboxes on features #56

merged 6 commits into from
Apr 28, 2022

Conversation

moradology
Copy link
Contributor

@moradology moradology commented Apr 26, 2022 •
edited
Loading

This PR addresses #55 by setting the default value of bbox and geometry on features to None

@moradology moradology force-pushed the fix/allow-none-geom+bbox branch from 057e865 to 3f6d11f Compare April 26, 2022 17:45
@codecov-commenter
Copy link

codecov-commenter commented Apr 26, 2022 •
edited
Loading

Codecov Report

Merging #56 (d051ad6) into master (79a7fb9) will not change coverage.
The diff coverage is 100.00%.

@@            Coverage Diff            @@
##            master       #56   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files            4         4           
  Lines          106       106           
=========================================
  Hits           106       106
Flag Coverage Δ
unittests 100.00% <100.00%> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
geojson_pydantic/features.py 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 79a7fb9...d051ad6. Read the comment docs.

geospatial-jeff
geospatial-jeff reviewed Apr 26, 2022
View reviewed changes
geojson_pydantic/features.py
@@ -16,10 +16,10 @@ class Feature(GenericModel, Generic[Geom, Props]):
"""Feature Model"""

type: str = Field("Feature", const=True)
geometry: Geom
geometry: Geom = None
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
geometry: Geom = None
geometry: Optional[Geom] = None

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree that's easier to read. Do you think I should remove this line and incorporate the suggested change?

geojson-pydantic/geojson_pydantic/features.py

Line 12 in 5475550

Geom = TypeVar("Geom", bound=Optional[Geometry])

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I opted for a via media: https://github.com/developmentseed/geojson-pydantic/pull/56/files#diff-a4fab2efe48317b94ba18803db374650b7685147f52df92c7998ad4176e4f675R12-R19

Copy link
Contributor

@geospatial-jeff geospatial-jeff Apr 26, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Whoops my apologies I forgot that the Optional was rolled into the TypeVar, i'd prefer it to remain in the TypeVar. My bad! 😬

Elaboration here: #47 (comment)

@moradology moradology force-pushed the fix/allow-none-geom+bbox branch from 7479650 to 04cad7f Compare April 26, 2022 19:07
@moradology
Copy link
Contributor Author

I think this is likely good to go. Are there any other outstanding changes we really want to see prior to another release? (Getting things lined up to push a release for stac-fastapi)

@vincentsarago vincentsarago self-requested a review April 28, 2022 20:39
@vincentsarago vincentsarago merged commit f5fbad4 into developmentseed:master Apr 28, 2022
@vincentsarago vincentsarago mentioned this pull request Apr 28, 2022
Closed
@vincentsarago
Copy link
Member

@moradology @geospatial-jeff I'm getting a pre-commit error over #62 related to change made here

geojson_pydantic/features.py:19: error: Incompatible types in assignment (expression has type "None", variable has type "Geom")
[68](https://github.com/developmentseed/geojson-pydantic/runs/6689482310?check_suite_focus=true#step:5:69)
Found 1 error in 1 file (checked 6 source files)

@moradology
Copy link
Contributor Author

This looks a bit like a bug, but perhaps there's some wrinkle to the behavior here I'm missing. As you can see, the type variable is declared to be an Optional, which ought to include None:

geojson-pydantic/geojson_pydantic/features.py

Line 12 in ad3aabe

Geom = TypeVar("Geom", bound=Optional[Geometry])

I'm happy to cut a PR or somesuch to make modifications that will avoid this issue. Do we have a sense about what the right approach is?

@vincentsarago
Copy link
Member

@moradology To be honest I not sure where this comes from so I'll be more than happy if you can have a look 🙏

@vincentsarago
Copy link
Member

@moradology you can ignore ☝️ a other PR (#64) seems to work 👌 so I guess it's just a mypy configuration issue in #62

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@geospatial-jeff geospatial-jeff geospatial-jeff approved these changes

@vincentsarago vincentsarago vincentsarago approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@moradology @codecov-commenter @vincentsarago @geospatial-jeff