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 Geometry discriminator #101

Merged

Conversation

eseglem
Copy link
Collaborator

@eseglem eseglem commented Jan 31, 2023

What I am changing

  • Add discriminator to Geometry to check less types, and simplify errors.

How I did it

  • Annotated it.

How you can test it

  • Existing tests still work.

Related Issues

Unfortunately this PR does not do the second part, so I didn't way to say it fixes the issue entirely.

class Feature(GenericModel, Generic[Geom, Props]):
    ...
    geometry: Annotated[Union[Geom, None], Field(..., discriminator="type")]

Causes errors in the tests when the generic is overridden with a single Geometry type. It results in TypeError: 'discriminator' can only be used with 'Union' type with more than one variant

@vincentsarago vincentsarago self-requested a review February 1, 2023 15:58
@vincentsarago
Copy link
Member

Causes errors in the tests when the generic is overridden with a single Geometry type. It results in TypeError: 'discriminator' can only be used with 'Union' type with more than one variant

I'm not sure to understand. Do this PR resolves the issue? Will it work for Generic? The tests seems to pass so I don't really get it

@eseglem
Copy link
Collaborator Author

eseglem commented Feb 1, 2023

@vincentsarago Sorry, I was not very clear there. This PR at least improves upon the issue. As it stands there are no errors in the tests. I would have liked to have completely fixed it but that may not be possible.

This PR only implements the first comment I made on the issue. That results in only 3 validation errors instead of 18:

pydantic.error_wrappers.ValidationError: 3 validation errors for Feature
geometry -> Polygon -> coordinates
  All linear rings have the same start and end coordinates (type=value_error)
geometry -> type
  unexpected value; permitted: 'GeometryCollection' (type=value_error.const; given=Polygon; permitted=('GeometryCollection',))
geometry -> geometries
  field required (type=value_error.missing)

This PR does not implement the second comment I made. I tried to an additional discriminator to Feature.geometry and get it down to a single validation error. Unfortunately, when I ran the tests they threw errors in any case where Feature[] is given a single Geometry. For example Feature[Polygon, dict] will throw: TypeError: 'discriminator' can only be used with 'Union' type with more than one variant. It may be possible to get around this but I have not yet figured out the way.

So I figured it would be better to push the PR as it stands. It is still a significant improvement, even if it isn't the ideal outcome of only 1 validation error.

@vincentsarago vincentsarago merged commit b93b2b6 into developmentseed:main Feb 9, 2023
@eseglem eseglem deleted the feature/geometry-discriminator branch February 10, 2023 14:17
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.

2 participants