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

Fix: geoalchemy2.functions type stubs #481

Merged
merged 2 commits into from
Oct 21, 2023

Conversation

mbway
Copy link
Contributor

@mbway mbway commented Oct 20, 2023

Description

I made a mistake when writing the function stub generator. I should have familiarised myself with geoalchemy2 first (I have never used it before, I contributed the types to improve type checking coverage in a codebase that I work with).

The dynamic 'functions' in the geoalchemy2.functions module are only markers to be used to construct SQL queries and do no computation themselves and thus the type hints were incorrect. I knew they served the SQL construction purpose but for some reason I was under the impression that they could also be used as regular functions eg

geom: WKBElement = ST_GeomFromText('POINT(0, 0)')
geom2: WKBElement = ST_Buffer(geom, 1)

but this is not the case.

I took a look at how functions are type hinted in SQLAlchemy, for example the SQLAlchemy documentation explains how to create a custom generic function:

class as_utc(GenericFunction):
    type = DateTime()
    inherit_cache = True

So the geoalchemy functions should look like this.

There is a more specific way these can be typed, quoting the latest SQLAlchemy source code:

Type parameters for [GenericFunction] as a
`generic type <https://peps.python.org/pep-0484/#generics>`_ can be passed
and should match the type seen in a :class:`_engine.Result`. For example::

        class as_utc(GenericFunction[datetime.datetime]):
            type = DateTime()
            inherit_cache = True

The above indicates that the following expression returns a ``datetime``
object::

        connection.scalar(select(func.as_utc()))

but this is not available in SQLAlchemy 1.4 which GeoAlchemy2 has as the minimum version and I also wasn't sure what to put as the value for GenericFunction[T]

Checklist

This pull request is:

  • A documentation / typographical error fix
    • Good to go, no issue or tests are needed
  • A short code fix
    • Please include: Fixes: #<issue number> in the description if it solves an existing issue
      (which must include a complete example of the issue).
    • Please include tests that fail with the main branch and pass with the provided fix.
  • A new feature implementation or update an existing feature
    • Please include: Fixes: #<issue number> in the description if it solves an existing issue
      (which must include a complete example of the feature).
    • Please include tests that cover every lines of the new/updated feature.
    • Please update the documentation to describe the new/updated feature.

@mbway mbway mentioned this pull request Oct 20, 2023
@mbway
Copy link
Contributor Author

mbway commented Oct 20, 2023

I think this wasn't caught because the tests don't actually import or use anything from geoalchemy2.functions so there was nothing for the type checker to reject

@adrien-berchet
Copy link
Member

I think this wasn't caught because the tests don't actually import or use anything from geoalchemy2.functions so there was nothing for the type checker to reject

That's possible indeed. Could you add a new test for this that fails with the main branch and passes with this PR plz?

@adrien-berchet
Copy link
Member

Thanks for this quick fix anyway! :)

@mbway
Copy link
Contributor Author

mbway commented Oct 21, 2023

I have added a simple sanity check unit test that ensures that a simple query formed using geoalchemy2.functions is well typed. On the master branch, mypy catches the error with:

error: No overload variant of "select" matches argument type "Geometry"  [call-overload]

but on this branch everything is OK.

with the previous type stubs the return value of geoalchemy2.functions.ST_Buffer(geom, 1.0) was geoalchemy2.types.Geometry which is not what sqlalchemy.select is expecting.

@adrien-berchet
Copy link
Member

Ok, I see, thanks!
Let's merge this one, then I'll rebase #480 so you can test it for you own use cases before I release it.

@adrien-berchet adrien-berchet merged commit ee5b5ad into geoalchemy:master Oct 21, 2023
8 checks passed
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