-
Notifications
You must be signed in to change notification settings - Fork 2
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
Edr feature collection #7
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #7 +/- ##
==========================================
+ Coverage 97.95% 98.02% +0.07%
==========================================
Files 10 11 +1
Lines 196 203 +7
==========================================
+ Hits 192 199 +7
Misses 4 4
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
3c76822
to
8a3b259
Compare
|
||
from edr_pydantic.base_model import EdrBaseModel | ||
from edr_pydantic.parameter import Parameter | ||
from geojson_pydantic import FeatureCollection # type: ignore |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we need a # type: ignore
here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because mypy is not able to find the stubs for the external packages. I would have to look into it, but I think that there are two possible fixes:
- add
py.typed
to the library and ensure it is distributed by adding it to pytoml or setup.py:
[tool.setuptools.package-data]
"<package_name>" = ["py.typed"]
- Ignore it.
from typing import Dict | ||
|
||
from edr_pydantic.base_model import EdrBaseModel | ||
from edr_pydantic.parameter import Parameter |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It should be covjson_pydantic.parameter
!
I think the parameter
inconsistency between EDR and CovJSON should be corrected in EDR v1.2:
opengeospatial/ogcapi-environmental-data-retrieval#427
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed
"parameters": { | ||
"tx_dryb_10": { | ||
"type": "Parameter", | ||
"description": "Temperature, air, maximum, 10", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should use i18n
here and in several other places.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed
@@ -19,8 +19,8 @@ classifiers = [ | |||
"Topic :: Scientific/Engineering :: GIS", | |||
"Typing :: Typed", | |||
] | |||
version = "0.2.1" | |||
dependencies = ["pydantic>=2.3,<3"] | |||
version = "0.3.0" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this a breaking change?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, when upgrading you can still use the FeatureCollection of geojson-pydantic. It is not mandatory to use the model or implement the parameters.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Then why bump minor version?
Co-authored-by: Paul van Schayck <[email protected]>
15314f9
to
c2ffcbe
Compare
The focus of regular GeoJSON is representing spatial data. EDR GeoJSON adds custom properties that help users discover what is available in the request. See here for the optional requirements.
The GeoJSON package implements the GeoJSON standards. The models inside the package ignore extra properties. Therefore it cannot be used to add the optional properties of EDR Feature Collection GeoJSON. In addition, I think it is nice to have validation on the parameters which a custom model makes possible.