Add type annotations

[shuttle1987]

These changes add in type annotations to the code.

[adrien-berchet]

Very nice, thanks for this PR!!!

[adrien-berchet]

Another thing: it could be nice to add some tests with mypy to check the annotations are correct.

[adrien-berchet]

Also, I think we have to add a py.typed file, right? https://peps.python.org/pep-0561/#packaging-type-information

[shuttle1987]

As far as I understand py.typed is for when you are distributing type stub files. Because we aren't supporting Python 2 I don't know if we need this?

[shuttle1987]

I'm not so familiar with the CI pipeline being used for this project but I'm curious as to why the linting step in tox fails only for python==3.8?

[adrien-berchet]

As far as I understand py.typed is for when you are distributing type stub files. Because we aren't supporting Python 2 I don't know if we need this?

I'm not sure either, I never setup type hints for a library that will be used in other codes. Did you try to run Mypy on another code that includes GeoAlchemy2 objects?

I'm not so familiar with the CI pipeline being used for this project but I'm curious as to why the linting step in tox fails only for python==3.8?

It's just because the lint tox job is run only in the Py38 CI job.

[shuttle1987]

That makes sense about the single job failing if that's the only place the liniting is run. I've pushed a couple of commits to attempt to deal with the linter complaints

[adrien-berchet]

Sorry I should have tell you that before but you can run tox r -e format locally to fix most of the simple linting issues.

[adrien-berchet]

Now it's good, thanks :⁠-⁠)
Do you want to add a few tests for these annotations? Or I can do it later if you prefer. I never tried it but I think we can use this pytest plugin: https://pypi.org/project/pytest-mypy-testing/ . But it can be something else if you know a better solution.

[shuttle1987]

I'm not familiar with that package, in other projects I've added a mypy check as part of the CI run.

One other question I had was the types found in geoalchemy2/comparator.py, do we want to annotate return types there? I wasn't sure exactly what those types were supposed to be so I didn't annotate them in this PR.

[adrien-berchet]

For comparators I'm not sure. We should check the type annotations in SQLAlchemy and do the same.

[adrien-berchet]

For comparators I'm not sure. We should check the type annotations in SQLAlchemy and do the same.

It seems that the comparators can get a WKT str, a WKTElement, or a WKBElement and they return a sqlalchemy.sql.expression.BinaryExpression.

[shuttle1987]

I'm not so sure how best to annotate the types of the Comparator class, are these changes worth merging as is? Or is anyone able to push a commit with a fix?

[adrien-berchet]

As far as I can see, the comparators should actually return sqlalchemy.sql.elements.ColumnElement. And their inputs should be Union[str, WKTElement] (and maybe also WKBElement but I'm not sure?)
Also, could you try to add tests using pytest-mypy please? So we are (almost) sure that annotations are correct.

[adrien-berchet]

Looks great! Now I think we just have to fix mypy tests, do you need help for it?

[shuttle1987]

There was some missing type stub files in the last run, I've attempted to install those in 98d592e

I think there's a few things that will fail however, but I'd like to see what those are from the CI runner. I suspect that we will need to add to the tox configuration some mypy options including the following:

# MYPY
[tool.mypy]
ignore_missing_imports = true

[adrien-berchet]

There was some missing type stub files in the last run, I've attempted to install those in 98d592e

Nice!

I think there's a few things that will fail however, but I'd like to see what those are from the CI runner. I suspect that we will need to add to the tox configuration some mypy options including the following:

# MYPY
[tool.mypy]
ignore_missing_imports = true

Yeah indeed we will probably have to add some custom config for mypy.

[shuttle1987]

Looks like my idea of using mypy --install-types --non-interactive isn't going to work nicely inside a CI run since there's no cache.

[shuttle1987]

Looks like we encounter this issue: sqlalchemy/sqlalchemy2-stubs#242

[adrien-berchet]

Looks like we encounter this issue: sqlalchemy/sqlalchemy2-stubs#242

Since this issue is supposed to be fixed in SQLAlchemy>=2 I suggest we run Mypy only for the *-sqlalatest jobs. WDYT?

[shuttle1987]

If you could change the mypy test run to only run vs SQLAlchemy >= 2 that would be great.

[adrien-berchet]

In the end I could make it work for both sqlalchemy==1.4.* and sqlalchemy>=2.
I fixed several things to make Mypy happy and I added some tests for comparators to be sure. Does it look good to you?

[shuttle1987]

Overall this looks good to me.

[adrien-berchet]

Thank you very much for this work @shuttle1987 !
I think I will push a new release soon to make this available.

[adrien-berchet]

@shuttle1987 I just released the 0.14.1 version, so now you can use Mypy with GeoAlchemy 2 🎉