malthe / chameleon

Fast HTML/XML template engine for Python
https://chameleon.readthedocs.io
Other
177 stars 64 forks source link

Adds type hints to public API. #407

Closed Daverball closed 9 months ago

Daverball commented 10 months ago

Closes #405

I tried to keep the diff relatively small, but I also wanted to enable a relatively strict mypy config for the public API, so I ended up annotating a bit more to guarantee a robust basis.

I also ran autotyping once which uses black to auto format the code, so there are a few formatting changes that have nothing to do with type annotations, but I made sure to revert most of them, to keep the diff as small as possible.

I also removed some of the leftover Python 2.7 compatibility stuff in the places where mypy wasn't happy about it.

I added a mypy environment to the tox config and added it to the matrix for github actions.

This also adds a minimal pyproject.toml. Some of the tool configuration could be moved from setup.cfg to pyproject.toml but for now it's probably not worth the effort, especially since flake8 doesn't support pyproject.toml yet and tox only sort of supports it by pasting the entire tox.ini in a multiline string inside the pyproject.toml.

malthe commented 10 months ago

Somewhat unrelated, but from Python 3.9, we have https://docs.python.org/3.9/library/ast.html#ast.unparse – might be useful to avoid the AST code generation code altogether.

malthe commented 10 months ago

The import typing as t pattern – doesn't that potentially shadow a variable t in a way that T wouldn't, i.e. T.Optional – ?

But alternatively, why not just import Optional, Mapping and friends explicitly?

Daverball commented 10 months ago

The import typing as t pattern – doesn't that potentially shadow a variable t in a way that T wouldn't, i.e. T.Optional – ?

But alternatively, why not just import Optional, Mapping and friends explicitly?

I mean it's a top level import, if any shadowing was going on it would shadow the other way, and if it did and had any sort of effect on the type annotations, then type checkers would complain, loudly.

I like this style for <3.10 because it avoids having a ton of typing specific imports. Starting with 3.9 and 3.10 you can get rid of a lot of those imports for the new union syntax and subscriptable builtin types and ABCs.

If you don't mind a from __future__ import annotations import we may be able to get away with 3.10 style annotations, in that case I wouldn't mind doing separate imports for symbols from typing.

coveralls commented 10 months ago

Pull Request Test Coverage Report for Build 7346414880


Changes Missing Coverage Covered Lines Changed/Added Lines %
src/chameleon/astutil.py 88 89 98.88%
src/chameleon/tal.py 8 10 80.0%
src/chameleon/template.py 45 47 95.74%
src/chameleon/zpt/template.py 41 43 95.35%
<!-- Total: 392 399 98.25% -->
Files with Coverage Reduction New Missed Lines %
src/chameleon/template.py 1 90.73%
src/chameleon/zpt/loader.py 1 84.38%
<!-- Total: 2 -->
Totals Coverage Status
Change from base Build 7314528887: 0.2%
Covered Lines: 4325
Relevant Lines: 4813

💛 - Coveralls
malthe commented 10 months ago

I think we could probably bump up to Python 3.10 to benefit more from this typing endeavor – if not in this pull request, then perhaps after the next release.

Daverball commented 10 months ago

I think a bump to 3.10 is maybe a bit aggressive, but a bump to 3.9 seems reasonable considering 3.8's EOL is less than a year away, then you could also work on your unrelated improvement/simplification to code generation.

If we use deferred annotations using from __future__ import annotations, then we can use the new union syntax in most places where it is relevant and for type aliases and casts we can just wrap them in a string manually if we want to use it there too. This will also reduce the runtime overhead of type annotations. Even though it's mostly just an import time overhead, it is an overhead nevertheless, which some people may care about.

malthe commented 10 months ago

I don't know if for example the Python RPMs built for Chameleon are .pyc-files only (see #348), but they could be and that would take care of such an overhead for perhaps more enterprise-oriented users.

malthe commented 10 months ago

I think a bump to 3.10 is maybe a bit aggressive, but a bump to 3.9 seems reasonable considering 3.8's EOL is less than a year away

Yes, I think we could bump to 3.9.

Perhaps it's reasonable enough to cut a release now and then introduce typing in the next release, with a 3.9+ requirement to boot.

Daverball commented 10 months ago

I don't know if for example the Python RPMs built for Chameleon are .pyc-files only (see #348), but they could be and that would take care of such an overhead for perhaps more enterprise-oriented users.

The type annotations are also present in byte code, so they would cause overhead either way, as long as they're evaluated at runtime, the from __future__ import annotations would defer that evaluation until a runtime library tries to inspect the type annotations.

The majority of that overhead stems from generics, since things like list[int] invokes list.__class_getitem__ and creates an instance of typing._GenericAlias which is a proxy object for the original class which contains the additional information about the bound type parameters so that they can be inspected at runtime.

Daverball commented 10 months ago

I updated the type hints to the modern style. I also added a flake8 plugin which should help with minimizing the runtime overhead of type annotations. It will warn about things like imports that are only used in type annotations so they can be moved inside a if TYPE_CHECKING block or uses of cast that don't wrap their type in a string.

Daverball commented 10 months ago

I already had some stubs lying around for exc and utils which I started writing a while back, so I decided to include those as well. This uncovered a couple of smaller issues which I either decided to fix, when obvious, but otherwise I left a FIXME comment, we can address them in any way you want @malthe pending your review.

malthe commented 9 months ago

I'll merge this – let's iterate on the master branch on improvements and further changes.

malthe commented 9 months ago

See https://github.com/malthe/chameleon/releases/tag/4.5.0.