-
Notifications
You must be signed in to change notification settings - Fork 71
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
Type Geometry #173
Type Geometry #173
Conversation
lib/geo_postgis/geometry.ex
Outdated
@type t :: | ||
Point.t() | ||
| PointZ.t() | ||
| PointM.t() | ||
| PointZM.t() | ||
| LineString.t() | ||
| LineStringZ.t() | ||
| Polygon.t() | ||
| PolygonZ.t() | ||
| MultiPoint.t() | ||
| MultiPointZ.t() | ||
| MultiLineString.t() | ||
| MultiLineStringZ.t() | ||
| MultiPolygon.t() | ||
| MultiPolygonZ.t() |
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.
We could use some macro to generate this..
something like
@type t :: unquote(Enum.reduce(@geometries, fn g, acc -> {:|, [], [g, acc]} end))
but I'm not sure if it's better..
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.
Mmm, right... I do think we should build the type automatically from the @geometries
list, just so that the long-term maintenance doesn't depend on remembering to add new types in both places. We could potentially wrap the AST generation in a macro like this, but I'd be comfortable with the above inline unquote
with a comment explaining that it's producing the union of all the geometry types.
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.
hey! actually this is already declared as Geo.geometry() all we need is just to refer to it!
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.
Ahhh! Thanks for this, @RudolfMan! I'm excited to finally be able to get rid of the Dialyzer ignore rule I've been carrying for years. 😆
lib/geo_postgis/geometry.ex
Outdated
@type t :: | ||
Point.t() | ||
| PointZ.t() | ||
| PointM.t() | ||
| PointZM.t() | ||
| LineString.t() | ||
| LineStringZ.t() | ||
| Polygon.t() | ||
| PolygonZ.t() | ||
| MultiPoint.t() | ||
| MultiPointZ.t() | ||
| MultiLineString.t() | ||
| MultiLineStringZ.t() | ||
| MultiPolygon.t() | ||
| MultiPolygonZ.t() |
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.
Mmm, right... I do think we should build the type automatically from the @geometries
list, just so that the long-term maintenance doesn't depend on remembering to add new types in both places. We could potentially wrap the AST generation in a macro like this, but I'd be comfortable with the above inline unquote
with a comment explaining that it's producing the union of all the geometry types.
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.
Nice find! Getting the typing right here will be soo nice.
When using with
TypedEctoSchema
it tries to create a type for the schema as
And Geometry.t() is not declared.
Current workaround for us is declaring that type in our application
and refer to it in schema