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

Type Geometry #173

Merged
merged 2 commits into from
Sep 23, 2023
Merged

Type Geometry #173

merged 2 commits into from
Sep 23, 2023

Conversation

RudolfMan
Copy link
Contributor

When using with TypedEctoSchema

defmodule MyApp.Service do
  use TypedEctoSchema

  typed_schema "services" do
    field :location, Geo.PostGIS.Geometry
    field :foo, :integer
  end
end

it tries to create a type for the schema as

  @type t() :: %__MODULE__{
          location: Geo.PostGIS.Geometry.t()
          foo: integer()
          ...
        }

And Geometry.t() is not declared.

Current workaround for us is declaring that type in our application

defmodule MyApp.Geo.PostGIS.Geometry do
  @type t ::
            Geo.Point.t()
            | Geo.PointZ.t()
            ...
end

and refer to it in schema

typed_schema "services" do
  field(:location, Geo.PostGIS.Geometry) :: MyApp.Geo.PostGIS.Geometry.t()
  field :foo, :integer
end

Comment on lines 60 to 74
@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()
Copy link
Contributor Author

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..

Copy link
Collaborator

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.

Copy link
Contributor Author

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!

Copy link
Collaborator

@s3cur3 s3cur3 left a 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. 😆

Comment on lines 60 to 74
@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()
Copy link
Collaborator

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.

Copy link
Contributor

@axelson axelson left a 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.

@axelson axelson merged commit 7dcd892 into felt:master Sep 23, 2023
19 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants