test: implement type hints

[kuwv]

[EDITOR'S NOTE TO FUTURE SELF: I specifically requested that @kuwv poke at this so I could have a gander at what the codebase feels like with typing rubbed on it. See #904.]

[kuwv]

@bitprophet, @Dreamsorcerer the merge is near complete. The typing_extensions is 4.1.1 for compatibility with 3.6 but probably need to limit import to 3.6 and 3.7. Coverage percentage is off though.

There's a few fixes in code but some are commented instead:

./invoke/executor.py:                # FIXME: task.name can be none here
./invoke/parser/parser.py:            initial=self.initial,  # type: ignore # FIXME: should not be none
./invoke/loader.py:            # FIXME: deprecated capability that needs replacement
./invoke/completion/complete.py:        # FIXME: this seems wonky
./invoke/context.py:        # FIXME pattern should be raw string prompt.encode('unicode_escape')

If you or anyone else would like to see it:

python -m venv .venv
source .venv/bin/activate
pip install -r dev-requirements.txt
mypy .

Half of the benefits of using mypy is documenting the API but it requires setup with your IDE (or vim/ale).

[kuwv]

@bitprophet could you let us know what your thoughts are? I'd like to have @Dreamsorcerer feedback while we got him. Thanks!

[bitprophet]

Taking a look now, though my general feeling so far is positive 👍🏻

[Dreamsorcerer]

Alright, I'll pull it locally at the weekend and go through it fully.

[Dreamsorcerer]

@bitprophet Just looking at the vendor parts, not sure what your thoughts are, but I'm not too sure about vendoring typing_extensions. It seems to cause extra type errors and should be a pretty straightforward library to use as an external dependency (plus, can probably avoid installing entirely on recent versions of Python).

I'm also confused by this import: https://github.com/pyinvoke/invoke/blob/main/invoke/parser/context.py#L3-L6

Why would that import ever fail?

[bitprophet]

@Dreamsorcerer The odd looking try/except imports are because some downstream packagers (usually Linux distros) have Opinions™ about vendoring, and wanted the code to gracefully deal with situations where they've not distributed the vendor subtree, but have instead ensured via the package manager that external versions of those libraries are around.

I consider(ed) this to be in line with the vendoring itself, in a "neither of these solutions is elegant but in tandem they make it a lot easier for this tool to be trivially available as a CLI app for the most users" sense.

EDIT: so, re: typing_extensions, my assumption was that it falls under the other vendored packages, but I hadn't yet grokked exactly why it was brought in to begin with.

[bitprophet]

Note to self or @kuwv - before we merge this wants a changelog entry and I assume adding a CI task that runs mypy invoke/ or similar. Also, that command gives me a few more typing errors when I run it locally - not sure if that means I'm running something differently than you were?

[Dreamsorcerer]

@Dreamsorcerer The odd looking try/except imports are because some downstream packagers (usually Linux distros) have Opinions™ about vendoring, and wanted the code to gracefully deal with situations where they've not distributed the vendor subtree, but have instead ensured via the package manager that external versions of those libraries are around.

Right, makes sense. They will be fixed to specific versions anyway as part of the distro packaging, so it makes sense for them to use an external package which they know has relevant security fixes etc.

EDIT: so, re: typing_extensions, my assumption was that it falls under the other vendored packages, but I hadn't yet grokked exactly why it was brought in to begin with.

Why it was imported? That's because some types are not available in older releases, for example typing.Literal. If we don't want anything fancy (the newer typing things are more finetuning that anything fundamental), then we can probably have typing_extensions only for Python <= 3.8.

So, you'd end up with a few imports like:

if sys.version_info >= (3, 8):
    from typing import Literal
else:
    from typing_extensions import Literal

(mypy will understand this correctly with version_info compares, it won't like a try/except).

Note to self or @kuwv - before we merge this wants a changelog entry and I assume adding a CI task that runs mypy invoke/ or similar. Also, that command gives me a few more typing errors when I run it locally - not sure if that means I'm running something differently than you were?

I'm playing with the config at the moment, but one of the things I recommend is files = ..., then you only need to run mypy, no arguments, and you don't need to worry about remembering which paths to use in every place you call it.

[kuwv]

Note to self or @kuwv - before we merge this wants a changelog entry and I assume adding a CI task that runs mypy invoke/ or similar. Also, that command gives me a few more typing errors when I run it locally - not sure if that means I'm running something differently than you were?

Will do!

I'm running it on Python 3.10. I'll test is with each of the respective versions this weekend. Also, I plan on adding a pytest plugin by @dbader to run mypy with pytest https://pypi.org/project/pytest-mypy/. We can refactor it later but I'd like the CI pipeline to run mypy and this is will work.

edit: nixed the mypy plugin

[Dreamsorcerer]

I'm running it on Python 3.10. I'll test is with each of the respective versions this weekend.

Generally not necessary, you can probably just put python_version = 3.7 in the config and test it will work for older versions.

Also, I plan on adding a pytest plugin by @dbader to run mypy with pytest https://pypi.org/project/pytest-mypy/. We can refactor it later but I'd like the CI pipeline to run mypy and this is will work.

Not sure what the current CI looks like, but I've had bad experiences with these kind of plugins. It adds complexity, another dependency, and usually causes problems at some point. I'd recommend just adding it as a separate command in a lint job or similar (a Makefile can always be used to help developers run all the correct steps locally).

[Dreamsorcerer]

To make typing easier, and probably make the code a bit simpler, I think invoke/terminals.py could be split up. e.g. terminals_win.py and terminals_unix.py, with terminals.py just importing the relevant one. That would avoid all the extra platform checks and if statements through the module.

I'll just type ignore any errors for now, but something that might be worth someone looking at later.

[Dreamsorcerer]

Also just noticed that fluidity-sm hasn't been touched in 12 years. I think that's long reached the point where it would be a good idea to release your own fork or make it part of invoke without vendoring. :P

[kuwv]

Also just noticed that fluidity-sm hasn't been touched in 12 years. I think that's long reached the point where it would be a good idea to release your own fork or make it part of invoke without vendoring. :P

@dreamsorcerer I actually have an overhauled fork of fluidity which I initially named fluidstate and added typing to it. I later renamed it superstate after I decided it would be much better for my other work as a statechart. I am nearly finished with it. I plan on submitting a PR to add it later.

https://github.com/kuwv/python-superstate/tree/e50038db63afa9a91badbec1387dda7fb25f504e https://github.com/kuwv/python-superstate

[kuwv]

I'm running it on Python 3.10. I'll test is with each of the respective versions this weekend.

Generally not necessary, you can probably just put python_version = 3.7 in the config and test it will work for older versions.

Also, I plan on adding a pytest plugin by @dbader to run mypy with pytest https://pypi.org/project/pytest-mypy/. We can refactor it later but I'd like the CI pipeline to run mypy and this is will work.

Not sure what the current CI looks like, but I've had bad experiences with these kind of plugins. It adds complexity, another dependency, and usually causes problems at some point. I'd recommend just adding it as a separate command in a lint job or similar (a Makefile can always be used to help developers run all the correct steps locally).

Yeah, really was just looking for a simple way to align the coverage. Yeah, I think I'm going to skip it.

[Dreamsorcerer]

OK, finished going through it. Here are my suggested changes: https://github.com/Dreamsorcerer/invoke/commit/7a9a9ddd14a6e891276962b663ecdd362992dd98

I also started adding tests/ to the checks, but there are a lot of errors occurring (not sure if pytest does something to the classes, but there are lots of nested classes which then access attributes on themselves which only exist in the outer class, which doesn't make much sense).

Typing the tests is one of the only places that the public API will get tested, so I already found a few issues with the typing from that. I suggest we come back to tests/ in the next PR.

[Dreamsorcerer]

I've also added TODO comments for typing enhancements that can be added in a later Python version, or with typing_extensions (I'll leave it to you to decide whether you want to bring typing_extensions in again or not, seems we don't need it currently).

[kuwv]

I've also added TODO comments for typing enhancements that can be added in a later Python version, or with typing_extensions (I'll leave it to you to decide whether you want to bring typing_extensions in again or not, seems we don't need it currently).

Yeah, I'll probably add it again later when I rewrite https://github.com/pyinvoke/invoke/pull/698.

OK, finished going through it. Here are my suggested changes: Dreamsorcerer@7a9a9dd

I also started adding tests/ to the checks, but there are a lot of errors occurring (not sure if pytest does something to the classes, but there are lots of nested classes which then access attributes on themselves which only exist in the outer class, which doesn't make much sense).

Typing the tests is one of the only places that the public API will get tested, so I already found a few issues with the typing from that. I suggest we come back to tests/ in the next PR.

@Dreamsorcerer I had to take your earlier advice and pin mypy to 0.971 to work with Python3.6.

# pip install mypy==0.991
ERROR: Could not find a version that satisfies the requirement mypy==0.991 (from versions: 0.1, 0.11, 0.12, 0.13, 0.14, 0.15, 0.16, 0.17, 0.18, 0.19, 0.20, 0.21, 0.221, 0.222, 0.223, 0.224, 0.225, 0.226, 0.227, 0.228, 0.229, 0.230, 0.231, 0.232, 0.233, 0.234, 0.235, 0.236, 0.237, 0.238, 0.239, 0.240, 0.241, 0.250, 0.251, 0.252, 0.253, 0.254, 0.255, 0.256, 0.470, 0.471, 0.501, 0.510, 0.511, 0.520, 0.521, 0.530, 0.540, 0.550, 0.560, 0.570, 0.580, 0.590, 0.600, 0.610, 0.620, 0.630, 0.641, 0.650, 0.660, 0.670, 0.700, 0.701, 0.710, 0.711, 0.720, 0.730, 0.740, 0.750, 0.760, 0.761, 0.770, 0.780, 0.781, 0.782, 0.790, 0.800, 0.812, 0.900, 0.901, 0.902, 0.910, 0.920, 0.921, 0.930, 0.931, 0.940, 0.941, 0.942, 0.950, 0.960, 0.961, 0.971)
ERROR: No matching distribution found for mypy==0.991

@bitprophet Do you want to add typing to tests? It seems that there are some issues with it if we do. Also, can you take a peek at @Dreamsorcerer's work and lmk what you would like to see?

[Dreamsorcerer]

@Dreamsorcerer I had to take your earlier advice and pin mypy to 0.971 to work with Python3.6.

There's no reason to run mypy under Python 3.6. It's a static type checker, so the version of Python that is being run shouldn't affect any results. Just run it in the linter step with a more recent version (I assume you're not using an ancient version of flake8 because they no longer run under 3.6/7), and set the python_version in the config.

As of yesterday, the version can be pinned to 1.0.0.

[Dreamsorcerer]

I've also added TODO comments for typing enhancements that can be added in a later Python version, or with typing_extensions (I'll leave it to you to decide whether you want to bring typing_extensions in again or not, seems we don't need it currently).

Yeah, I'll probably add it again later when I rewrite #698.

Sure, but as before, I'd recommend not vendoring it if you bring it in. mypy special cases typing_extensions, and by pulling it into the repo, you lose that special case and require mypy to parse the whole thing, which seems to result in additional errors.

Do you want to add typing to tests? It seems that there are some issues with it if we do.

The main thing is just figuring out what is going on with those classes and whether it's worth simplifying them to something more intuitive. All the tests seem to be written like this:

class Foo:
    def foo(self): ...

    class InnerFoo:
        def test(self):
            self.foo()  # How can this be defined?

In normal Python code, that is clearly broken. If those tests are actually working, then pytest or something must be doing something weird to modify the classes.

[Dreamsorcerer]

An obvious solution would be to use normal inheritance:

class Foo:
    def foo(self): ...

class InnerFoo(Foo):
    def test(self):
        self.foo()

[kuwv]

An obvious solution would be to use normal inheritance:

class Foo:

    def foo(self): ...

class InnerFoo(Foo):

    def test(self):

        self.foo()

I'd like to wait on doing the tests since the tasks and argument need more attention.

Essentially, 'task' is a decorator wrapping another decorator. This is causing Callable => Task => Callable.

[Dreamsorcerer]

I'd like to wait on doing the tests since the tasks and argument need more attention.

That was my suggestion. It'd be too much to change tests in this PR.

Essentially, 'task' is a decorator wrapping another decorator. This is causing Callable => Task => Callable.

Think this is already done in my suggested changes?

[Dreamsorcerer]

Let me know if you want me to put those changes into a PR, so they can reviewed separately.

[bitprophet]

Hey @Dreamsorcerer, @kuwv - I'll try to review the rest of this, and the discussion, today. At a high level, +1 on waiting on tests (it's an idiosyncratic pytest plugin that I'm not currently willing to drop, so if it means they cannot be reasonably typechecked, then maybe that's just moot).

---

What exactly is the deal with typing_extensions? Is it something we can specify as part of the development-only dependency list (and perhaps add to some docs in case that's insufficient for folks relying on our type annotations incidentally)?

I like the idea of supporting typing, but not to the point where we'd grow our first non-vendored dependency solely to support typechecking. (Maybe as a setuptools extra, eg invoke[typed/typing/whatever]? That might be the best of both worlds?)

[kuwv]

Let me know if you want me to put those changes into a PR, so they can reviewed separately.

I've updated most of your suggestions. I'm looking at the 'task' library though. It's not using 'decorator' lib or 'wraps'. It needs to provide its metadata and the wrapped function but it's not doing it idiomatically.

I'll be traveling and I'll be out of pocket here and there.

[Dreamsorcerer]

I'm looking at the 'task' library though. It's not using 'decorator' lib or 'wraps'. It needs to provide its metadata and the wrapped function but it's not doing it idiomatically.

I probably wasn't looking too closely, it just looked like something that wraps a function (similar to asyncio.Task), so I didn't think about wraps(). If that's relevant, feel free to add it in, but not sure it's needed for typing anyway.

[Dreamsorcerer]

What exactly is the deal with typing_extensions?

Pretty much includes newer typing features for backwards compatibility (including some experimental features not yet in a Python release).

Typical use case is to do something like:

if sys.version_info >= (3, 8):
    from typing import Literal
else:
    from typing_extensions import Literal

Then you can add it as a dependency only for older Python releases, for example in aiohttp we only depend on it for Python 3.7: https://github.com/aio-libs/aiohttp/blob/3.9/setup.cfg#L56

As I said above, if you don't want to depend on it at all, I've put TODOs in that can be updated when you drop support for the preceding Python version. The TODOs improve the accuracy of typing, but they are not game-changing.

Is it something we can specify as part of the development-only dependency list (and perhaps add to some docs in case that's insufficient for folks relying on our type annotations incidentally)?

No, it is needed at runtime.

[Dreamsorcerer]

As I said above, if you don't want to depend on it at all, I've put TODOs in that can be updated when you drop support for the preceding Python version.

When I drop support for a Python version, I actually look through the code base with (e.g. when dropping Python 3.6):

fgrep -Rn -e '36' -e '3.6' -e '3,6' -e '3, 6'
fgrep -Rn -e '37' -e '3.7' -e '3,7' -e '3, 7'

Which helps me to find all the TODOs and version_info checks that can now be removed.

[bitprophet]

Yea, I tend to do similar, # TODO: nuke/change/etc when dropping Python 3.6, then ack -i "py(thon)? ?3\.6" or whatnot.

And more to the point, acknowledged, thanks for the explainer. Given this is about accuracy/gentle degradation of typing ability, and not "its lack makes typechecking impossible or completely pointless", I think I'm fine with the TODOS+eventual updating approach.

I'm also open to changing my approach if I really get the typing religion and/or we get a lot of folks using this initial release of support who then return with "argh the lack of what typing_extensions would enable, is really killing us here!".

[bitprophet]

FWIW given both of y'all have already put a lot of time into this and one of you, at least, is partly AFK - I'm likely to make some executive decisions and kick some version of this out the door without a ton more back/forth, so the community can try it out.

Especially since my read of annotations is that we can hone them over time without it being a huge headache for people (vs "real" APIs which can break folks pretty bad if they change unexpectedly). So both gradual extensions of the annotations & occasional tweaking seem doable w/o a lot of hand-wringing. (I do so love hand-wringing...)

[bitprophet]

Finished reviewing Jesse's changes, made a small pile of my own where I disagreed, and now rolling through Sam's suggestions-via-commit (which I expect to manually apply so it doesn't pull in the test suite changes). Thanks again for all of this!

[bitprophet]

Whoops, I see @kuwv doing some of the same thing this evening (pulling in Sam's changes). I'll stop what I'm doing and wait for a go-ahead 😂 I have a handful of my own changes - see https://github.com/pyinvoke/invoke/tree/906-int if interested. I'll have to rebase/cherrypick them on top of whatever is added in here.

I will note that so far I've liked Sam's changes except for the Generics stuff around the Task class and task decorator; which I continue to find pretty confusing. While the more (heh) generic uses of Any+Callable that Jesse started out with is, surely, a lot more broad than the use of the overload decorator + TypeVar/Generic, I find it much easier to read/grok offhand.

[Dreamsorcerer]

I will note that so far I've liked Sam's changes except for the Generics stuff in invoke.tasks, which I continue to find pretty confusing. While the more (heh) generic use of Any that Jesse started out with is, surely, a lot more broad than the use of the overload decorator + TypeVar/Generic, I find it much easier to read/grok offhand.

Let me know if you want to go over anything. Any just means dynamic typing, so we lose all typing benefits whenever you see that. So, any function passed into a task becomes untyped, which for such a generic object is rather undesirable.

The TypeVar is almost like a placeholder and just means you'll get the same type out as you put in (in this case the _T in the body defines the return type that comes from __call__()). So, with the TypeVar in place this will work:

def foo() -> str: ...

t = Task(foo)
result = t()
reveal_type(result)  # str

Without that, you'll get Any.

The overloads are just for the decorator, which is a little awkward because it can be used as @task or @task(), so it's just trying to define both cases. Admittedly, the arguments in the signature feel slightly mangled in order to make it work, but again if you don't do that then every single function you put that decorator on is going to become untyped.

[Dreamsorcerer]

I think the only thing I'm not entirely certain on is invoke.parser.argument.Argument, which is really rather complicated from a static typing perspective, and I'm not sure how precise the types are. Because it has things like value and raw_value, it can have a default value, it can also be None, it might be a list which then append values, and set_value() may cast or not cast; it's all rather complicated as to what the correct types might be...

[kuwv]

@bitprophet I decided I needed to provide my latest updates. Sam fixed a bunch of stuff I overlooked. I'll leave the rest for you to decide. Still need to tune argument and tasks though but that can wait.

[bitprophet]

invoke.parser.argument.Argument, which is really rather complicated from a static typing perspective

Arguments literally stand in for CLI flags - so like most other such libraries, the possibilities for what type they end up presenting as (and their default values, etc) are indeed extremely varied. I'm not sure offhand what the "right" way to do this would be (ie if one were to write Invoke from scratch with typing in mind). Probably a tree of subclasses that could each hold more specific "once you get a value out" type hints? ListArg, etc.

I'm happy to kick the can down the road a bit for the thornier corners here - I assume that just being able to run mypy on client codebases (without having to mark Invoke as entirely untyped, as they would have prior) will be a big step up for many folks, and as I noted upthread, I expect we'll streamline this in the future, both the bits that are already happily typed here, and the bits that are in question or Any'd.

[kuwv]

@bitprophet I think this might have gotten reverted.

[bitprophet]

Yea, that's messed up. Clearly lazygit is a little too powerful sometimes. Will try to get it back.

[bitprophet]

@kuwv Should be restored now. Suspect this was due to me trying to juggle changes between 2.0 and main, must have somehow reset main to be equal to a version of 2.0. The trouble with trying to keep even a single stable branch around for folks…