feat: add generics to feature properties #29

iwpnd · 2021-05-08T10:43:43Z

Hi!

First of all sorry for the unsolicited PR.

I wanted to start a project similar to this but luckily I researched up front. Something was missing for me, and I kinda jumped on the idea to add generics to feature properties and it turned out quite okay right away.

The idea is, that instead of using a Dict[Any, Any] for the feature properties, you can opt to pass a model to Feature that is then parsed just like e.g. the Geometry.

This allows you, among other things, to validate feature properties in a Feature. Even cooler, it is not even a must. You can use it just like you are doing right now, but give the option to also parse the properties.

Example:

from pydantic import BaseModel
from geojson_pydantic import Feature

class MyFeatureProperties(BaseModel):
    id: str
    description: str

feature = Feature[MyFeatureProperties](**{
  "type": "Feature",
  "geometry": {
    "type": "Polygon",
    "coordinates": [
      [
        [13.38272,52.46385],
        [13.42786,52.46385],
        [13.42786,52.48445],
        [13.38272,52.48445],
        [13.38272,52.46385]
      ]
    ]
  },
  "properties": {
    "id": "test",
    "description": "i will be parsed"
  }
})

print(isinstance(feature.properties, MyFeatureProperties))
>> True

Let me know what you think.

geospatial-jeff · 2021-05-08T13:24:19Z

I was not aware that pydantic supported generics, this is really cool, I think a good feature to add. #28 adds similar functionality for exposing the Feature's geometry type, but using pydantic's create_model instead. I think we should use either one or the other (generics / create_model), not both. Generics seem more explicit to me.

drewbo · 2021-05-10T15:05:42Z

Really like this and agree that this is preferable to the model factory pattern. cc: @vincentsarago for thoughts

vincentsarago · 2021-05-10T15:20:01Z

@iwpnd 🙏 I love unsolicited PR.

I like the idea of generics (even if I'm just learning about it). Just noticing that Pydantic generic is only supported with python >3.6 (https://pydantic-docs.helpmanual.io/usage/models/#generic-models)

I'm fine dropping support of python 3.6 but this will need bigger discussion cc @geospatial-jeff @drewbo

iwpnd · 2021-05-10T15:25:39Z

Up to you. :)

Let me know if I can help to move this forward or there's something else you want me to do.

geospatial-jeff · 2021-05-11T01:36:06Z

I'm also fine with dropping support for python 3.6

vincentsarago · 2021-05-11T14:54:39Z

@iwpnd I'm trying to update this PR, could you check if we are allowed to push to your branch 🙏

iwpnd · 2021-05-11T15:55:26Z

I allowed edits by the maintainer @vincentsarago

…ydantic into pr/29

codecov-commenter · 2021-05-11T17:04:48Z

Codecov Report

Merging #29 (ee805bc) into master (e5894cb) will increase coverage by 0.13%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master      #29      +/-   ##
==========================================
+ Coverage   93.61%   93.75%   +0.13%     
==========================================
  Files           4        4              
  Lines          94       96       +2     
==========================================
+ Hits           88       90       +2     
  Misses          6        6

Flag	Coverage Δ
unittests	`93.75% <100.00%> (+0.13%)`	⬆️

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

Impacted Files	Coverage Δ
geojson_pydantic/features.py	`89.65% <100.00%> (+0.76%)`	⬆️

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 e5894cb...ee805bc. Read the comment docs.

vincentsarago · 2021-05-11T18:14:56Z

#28 adds similar functionality for exposing the Feature's geometry type, but using pydantic's create_model instead. I think we should use either one or the other (generics / create_model), not both. Generics seem more explicit to me.

@geospatial-jeff I agree we shouldn't use both but I'm not sure there is an easy way to allow dynamic modelling for geometry and properties using Generics 🤷‍♂️

FYI: I'm going to un-approve this PR for now even if the whole code is A+ and it does what advertised, but because it is in conflict with #28

iwpnd · 2021-05-11T19:17:54Z

Not too sure what you're trying to achieve, but you can pass as many types as you want to Feature. If you can elaborate, maybe I can help out.

vincentsarago · 2021-05-11T19:21:18Z

@iwpnd so what we wanted with #28 was to create model that only validate Polygon Feature for example as explained in #20 (comment)

iwpnd · 2021-05-11T19:43:32Z

Okay. So something like that:

from typing import TypeVar, Generic, Dict
from pydantic.generics import GenericModel, BaseModel
from geojson_pydantic.geometries import Geometry, Polygon

Props = TypeVar('Props', bound=Dict)
Geom = TypeVar('Geom', bound=Geometry)

class Feature(GenericModel, Generic[Geom, Props]):
    type: str = 'Feature'
    geometry: Geom
    properties: Props
        
class MyFeatureProperties(BaseModel):
    id: str
    description: str

feature = Feature[Polygon, MyFeatureProperties](**{
  "type": "Feature",
  "geometry": {
    "type": "Polygon",
    "coordinates": [
      [
        [13.38272,52.46385],
        [13.42786,52.46385],
        [13.42786,52.48445],
        [13.38272,52.48445],
        [13.38272,52.46385]
      ]
    ]
  },
  "properties": {
    "id": "test",
    "description": "i will be parsed"
  }
})

Where this passes as you'd expect it to do.

And

feature = Feature[Polygon, MyFeatureProperties](**{
  "type": "Feature",
  "geometry": {
    "type": "Cheese",
    "coordinates": [
      [
        [13.38272,52.46385],
        [13.42786,52.46385],
        [13.42786,52.48445],
        [13.38272,52.48445],
        [13.38272,52.46385]
      ]
    ]
  },
  "properties": {
    "id": "test",
    "description": "i will be parsed"
  }
})

raises:

ValidationError: 1 validation error for Feature[Polygon, MyFeatureProperties]
geometry -> type
  unexpected value; permitted: 'Polygon' (type=value_error.const; given=Cheese; permitted=['Polygon'])

vincentsarago · 2021-05-11T20:11:50Z

yes but does Feature[MyFeatureProperties] work still ?

iwpnd · 2021-05-11T20:33:48Z

yes but does Feature[MyFeatureProperties] work still ?

Only like this.

class Feature(GenericModel, Generic[Geom, Props]):
    type: str = 'Feature'
    geometry: Geom
    properties: Props
        
class MyFeatureProperties(BaseModel):
    id: str
    description: str

feature = Feature[None, MyFeatureProperties](**{
  "type": "Feature",
  "geometry": {
    "type": "Polygon",
    "coordinates": [
      [
        [13.38272,52.46385],
        [13.42786,52.46385],
        [13.42786,52.48445],
        [13.38272,52.48445],
        [13.38272,52.46385]
      ]
    ]
  },
  "properties": {
    "id": "test",
    "description": "i will be parsed"
  }
})

You either have it for both, or don't use them at all and stick to Feature().

But this is kind off drifting away from the scope of this PR, I guess.

iwpnd · 2021-05-17T06:51:37Z

Do you want to move forward with this? I'd be happy to spend some more time to make this work for you. Otherwise I'd be closing this PR and create a fork for my use-case.

vincentsarago · 2021-05-17T12:55:27Z

Hi @iwpnd
Sorry to let this slip through, I had other priorities last week. I'll try to work on this this week but I can't make promises. If you can't wait go ahead and create a fork 👍

iwpnd · 2021-05-18T06:44:42Z

Hi @vincentsarago, re-reading my comment I didn't mean to come across as pushy as it turned out. If you're saying you're still considering it, I'm okay.

vincentsarago · 2021-05-20T17:50:57Z

@iwpnd so I'm trying to add Generic for Geom and Props as

Props = TypeVar('Props', bound=Dict)
Geom = TypeVar('Geom', bound=Geometry)


class Feature(GenericModel, Generic[Geom, Props]):
    """Feature Model"""

    type: str = Field("Feature", const=True)
    geometry: Geom
    properties: Optional[Props]
    id: Optional[str]
    bbox: Optional[BBox]

Feature[Polygon, None](**f)
>>> ValidationError: 1 validation error for Feature[Polygon, NoneType]
properties
  value is not None (type=type_error.not_none)

Feature[None, MyFeatureProperties](**f)
>>> ValidationError: 1 validation error for Feature[NoneType, MyFeatureProperties]
geometry
  value is not None (type=type_error.not_none)

To be able to use MyFeatureProperies it seems to me that you'll have to pass geojson_pydantic.geometry.Geometry as the first input.

IMO this is not optimal, and create_model might be easier (because you can have defaults)

cc @geospatial-jeff

vincentsarago · 2021-05-20T18:48:46Z

After talking with @geospatial-jeff (on slack) I agree that's it's ok to being forced to provide both types (geom and props)

feat = Feature[Polygon, Dict]
feat = Feature[Geom, MyPropsModel]

I'm going to update this PR and wait for feedback

vincentsarago · 2021-05-20T18:55:01Z

I have added Geometry generic in addition to the Props generic.

Please 👍 👎

cc @geospatial-jeff @drewbo @iwpnd

iwpnd added 2 commits May 8, 2021 12:17

feat: add generics to feature properties

b25c296

fix: bind TypeVar to Dict type

0f9a031

vincentsarago mentioned this pull request May 11, 2021

drop python 3.6 and update CI #31

Merged

Merge branch 'master' of https://github.com/developmentseed/geojson-p…

ee805bc

…ydantic into pr/29

vincentsarago mentioned this pull request May 11, 2021

add model factory #28

Closed

vincentsarago self-requested a review May 11, 2021 17:06

vincentsarago approved these changes May 11, 2021

View reviewed changes

vincentsarago self-requested a review May 11, 2021 18:15

add geometry generic

74418dc

vincentsarago approved these changes May 21, 2021

View reviewed changes

vincentsarago merged commit c5bae4b into developmentseed:master May 21, 2021

vincentsarago mentioned this pull request May 21, 2021

add model factory? #20

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add generics to feature properties #29

feat: add generics to feature properties #29

iwpnd commented May 8, 2021

geospatial-jeff commented May 8, 2021 •

edited

Loading

drewbo commented May 10, 2021

vincentsarago commented May 10, 2021

iwpnd commented May 10, 2021

geospatial-jeff commented May 11, 2021 •

edited

Loading

vincentsarago commented May 11, 2021

iwpnd commented May 11, 2021

codecov-commenter commented May 11, 2021 •

edited

Loading

vincentsarago commented May 11, 2021

iwpnd commented May 11, 2021

vincentsarago commented May 11, 2021

iwpnd commented May 11, 2021

vincentsarago commented May 11, 2021

iwpnd commented May 11, 2021 •

edited

Loading

iwpnd commented May 17, 2021

vincentsarago commented May 17, 2021

iwpnd commented May 18, 2021

vincentsarago commented May 20, 2021

vincentsarago commented May 20, 2021

vincentsarago commented May 20, 2021

feat: add generics to feature properties #29

feat: add generics to feature properties #29

Conversation

iwpnd commented May 8, 2021

geospatial-jeff commented May 8, 2021 • edited Loading

drewbo commented May 10, 2021

vincentsarago commented May 10, 2021

iwpnd commented May 10, 2021

geospatial-jeff commented May 11, 2021 • edited Loading

vincentsarago commented May 11, 2021

iwpnd commented May 11, 2021

codecov-commenter commented May 11, 2021 • edited Loading

Codecov Report

vincentsarago commented May 11, 2021

iwpnd commented May 11, 2021

vincentsarago commented May 11, 2021

iwpnd commented May 11, 2021

vincentsarago commented May 11, 2021

iwpnd commented May 11, 2021 • edited Loading

iwpnd commented May 17, 2021

vincentsarago commented May 17, 2021

iwpnd commented May 18, 2021

vincentsarago commented May 20, 2021

vincentsarago commented May 20, 2021

vincentsarago commented May 20, 2021

geospatial-jeff commented May 8, 2021 •

edited

Loading

geospatial-jeff commented May 11, 2021 •

edited

Loading

codecov-commenter commented May 11, 2021 •

edited

Loading

iwpnd commented May 11, 2021 •

edited

Loading