Add ToolCache typing

[rmartin16]

Changes

Added typing support for ToolCache for QoL.
Specifically:

• Enumerate each Tool and its type in ToolCache
• Type tools as ToolCache each place it is introduced in a Tool

Rationale

Personally, I really like my IDE's hints and auto-completion. The current implementation of ToolCache prevents any of these luxuries because its attributes cannot be inferred.

• This is most noticeable for tools in briefcase.integrations because self.tools cannot be inferred as a ToolCache.
• Within commands, self.tools is at least inferred to be a ToolCache (at least for the subclasses that aren't mix-ins) but since none of the Tools are actually defined within ToolCache, none of them can be inferred.

Tradeoff

Unfortunately, this typing requires jumping through a few hoops and a bit more maintenance due to new dependencies within ToolCache itself....but I think its ultimately a win.

Also interested to know if this is just a PyCharm problem or if VS is in the same boat.

PR Checklist:

• All new features have been tested
• All new features have been documented
• I have read the CONTRIBUTING.md file
• I will abide by the code of conduct

[rmartin16]

Explanation of the Hoop Jumping

The primary problem with typing for ToolCache is circular imports. To avoid this, we have to make sure none of the typing is evaluated at runtime.

The from __future__ import annotations import makes all typing annotations be interpreted as strings. So android_sdk: AndroidSDK is interpreted as android_sdk: "AndroidSDK", for example. This avoids the requirement to actually import these definitions during runtime just for typing.

The if TYPE_CHECKING: <do imports> prevents actually doing the imports when Briefcase runs but allows type checkers and IDE's know what the types are.

Feedback is welcome.

[freakboy3742]

FWIW: Visual Studio will make use of type annotations if they're available, but doesn't get bent out of shape if they're missing. Anecdotally, my observation is that PyCharm is a lot more enthusiastic (or noisy, your pick 😄 ) about them.

I'm weakly in favour of adding type annotations - they can be good documentation, and as the codebase grows, they're another piece reinforcing correctness. My concern is the extent to which they become onerous to work with - adding a simple type declaration is mostly harmless; but requiring Optional[List[Dict[Tuple(... makes code harder to write, and I'm not sure it really improves readability, either.

The other thing to consider is whether we're treating this as "light documentation assistance", or actual type correctness. If we're going to pursue type annotation much further, I'd like to see some CI validation of the type declarations - there's not much point having them if they're wrong :-) My concern on this front is whether adding mypy on a partially typed codebase is going to cause more headaches than benefits; and whether the type of dynamic attribute management that we're doing with AppConfig is going to lead to contributors playing mypy whack-a-mole with every contribution.

[rmartin16]

@freakboy3742,
My primary impetus for this change is to avoid the common workflow I have now. For instance, I want to use a method from os but I can't remember its name. So I literally have to write import os; os.<blah> to find it, and then I can use it with self.tools.os. This extrapolates to everything in ToolCache though because nothing can have its type inferred.

This is purely efficiency for me.

As for actually enforcing typing.....I'm less interested in that goal....or in achieving some sort of typing coverage. To that end, I have been experimenting with typing for Command and AppConfig. I haven't landed on a good strategy yet though that isn't overly onerous to maintain.

[freakboy3742]

That makes sense; and it's certainly a lot less onerous to add typing in a few places where it's helpful than to do a wholesale type strictness change.

However, the question about long term maintenance still holds. What (if anything) can we do to ensure these type declarations are accurate, and stay that way? In particular, it makes me a little twitchy that we need to add chunks of code that are marked #pragma nocover just to keep an IDE happy; looking forward, if a new tool is added, there's nothing other than goodwill to ensure that any additional type declarations are added.

That's not necessarily a blocker from my perspective; I just want to make sure we've exhausted any possible safety mechanisms first.

[rmartin16]

I added this after discovering Ned said it should be part of the default exclude.

I'll revert back to # pragma: no cover if you're not a fan.

[freakboy3742]

I'd actually prefer it here; manual #pragma calls are sometimes inevitable, but are a code smell IMHO. My comment was more about having a fairly big block of code that isn't ever executed that only exists to keep an IDE happy, and the resulting problems ensuring that code is correct.

[rmartin16]

At first I was thinking testing these annotations is probably overkill....but then I discovered I spelled WiX wrong and quickly rethought my position 🤦🏼

This sent me down quite a few rabbit holes before getting here.

• I was able to reduce TYPE_CHECKING use to only integrations.base to prevent circular import errors.
• There are now tests to check if the types listed in ToolCache are correct.
• Tests will also catch if a tool is added to ToolCache without it being typed.
• I took the first step towards implementing the Tool base class....which is something I'd like to finish in the soonish future.
• To avoid a lot of hassle with Download and Subprocess, I moved their initialization out of ToolCache and in to BaseCommand.

[freakboy3742]

These mostly look like good improvements; a couple of questions/clarifications inline.

I'm not sure the tests are really testing that much, though. They're all tests of strings - so all we're really validating is that we're making the same typo in multiple locations :-) The only way I can think of improving this would be do walk over briefcase.integrations.__dict__ - that would be a list of actual module names, so __name__ would be reliably accurate.

[freakboy3742]

I'd actually prefer it here; manual #pragma calls are sometimes inevitable, but are a code smell IMHO. My comment was more about having a fairly big block of code that isn't ever executed that only exists to keep an IDE happy, and the resulting problems ensuring that code is correct.

[freakboy3742]

I presume this is an interim measure until we can convert tools like Xcode into full Tool classes?

[rmartin16]

Not necessarily. This idiom allows IDEs (and type checkers) to track a specific type being used in a higher level function that may operate on many types. In this case, it explicitly hints that the return value of ToolCache.add_tool(name=..., tool=...) will be the same as the type passed as tool.

Furthermore, though, ModuleType is necessary because tools.git is not a class; it is an actual module. Right now, TOOL_T isn't even accommodating xcode and xcode_cli.

[freakboy3742]

This strikes me as an odd construction - is there a reason for using this instead of the previous tools.mytool = mytool; return mytool? Unless we're planning to do additional verification on tools when they're added, I'm not sure I see what we gain over simple attribute assignment.

[rmartin16]

This is just for evaluating things added as tools during testing so every tool could be confirmed as typed in ToolCache. It doesn't serve a functional purpose otherwise.

[freakboy3742]

I'm not sure I follow how that validation works (or how it's a guarantee) - could you elaborate?

On top of that - my concern would be that add_tool is sufficiently un-idiomatic that the next tool added will inadvertently use tool.mytool = ..., and as a result, won't get picked up in testing.

[rmartin16]

I created ProxyToolCache in tests.integrations.conftest to inspect if tools added through ToolCache.add_tool() are annotated in ToolCache.

I agree it is straightforward to skip using this wrapper altogether; it would ultimately depend on devs and the reviewers enforcing it.

For the record, I'm not a big fan either of this either given the architecture of ToolCache....I just couldn't think of anything else to try to meet your reasonable comment below:

looking forward, if a new tool is added, there's nothing other than goodwill to ensure that any additional type declarations are added.

[rmartin16]

These mostly look like good improvements; a couple of questions/clarifications inline.

I'm not sure the tests are really testing that much, though. They're all tests of strings - so all we're really validating is that we're making the same typo in multiple locations :-) The only way I can think of improving this would be do walk over briefcase.integrations.__dict__ - that would be a list of actual module names, so __name__ would be reliably accurate.

We're a bit on the verge of rolling our own type checker... It may be possible to iterate though the namespace of briefcase.integrations but there are going to be a lot of caveats to which classes are relevant and which are not.

I think given the low volume and low churn of tools, we can confirm this typing is accurate; if not, I know PyCharm will definitely let people know.

While ToolCache.add_tool() is basically superfluous....I couldn't think of another way to 100% identify tools that aren't typed in ToolCache when they should be. I'm concerned that introspection of classes in briefcase.integrations could lead to false positives and largely irrelevant noise.

I'm not quite sure how far unaligned we may be...but I'm definitely willing to get on a call if it'd be at all useful to hash out these specifics as well as larger perspectives about typing. Like I mentioned, I would also like to institute a bit more typing for AppConfig and Command....but only if it fits with the within a reasonable maintenance lift.

[freakboy3742]

These mostly look like good improvements; a couple of questions/clarifications inline.
I'm not sure the tests are really testing that much, though. They're all tests of strings - so all we're really validating is that we're making the same typo in multiple locations :-) The only way I can think of improving this would be do walk over briefcase.integrations.__dict__ - that would be a list of actual module names, so __name__ would be reliably accurate.

We're a bit on the verge of rolling our own type checker... It may be possible to iterate though the namespace of briefcase.integrations but there are going to be a lot of caveats to which classes are relevant and which are not.

To be clear - I definitely don't want to go down the path of rolling our own. The __dict__ suggestion was mostly spitballing on my part; if there was an easy way to use "actual types" in the tests, rather than strings, then it seemed like an easy win. However, if there's no easy win, then I'm definitely not going to advocate for building massive amounts of tooling to validate a list of 10 strings that won't change that often.

I think given the low volume and low churn of tools, we can confirm this typing is accurate; if not, I know PyCharm will definitely let people know.

My concern is that something is committed to main incorrect, but it won't get picked up until someone with a specific editor tries to use the code. Given how often that list will change, maybe eyeballing it is enough.

I'm not quite sure how far unaligned we may be...but I'm definitely willing to get on a call if it'd be at all useful to hash out these specifics as well as larger perspectives about typing. Like I mentioned, I would also like to institute a bit more typing for AppConfig and Command....but only if it fits with the within a reasonable maintenance lift.

I don't think we're that far misaligned - I'm mostly pushing around the edges of this because (a) I don't use PyCharm, so I'm not experiencing the underlying problem that is motivating this change, and (b) generally speaking, I'm not a fan of changes that can't be (or aren't) validated by automated testing.

That said - practicality also beats purity. At this point, the only things that really stand out to me are the big "ignored" import block in integrations/base.py, and the architectural oddity of add_tools(). I can live with the former; and if the latter is a case of accepting an architectural oddity and getting weak testing, or not getting testing, then honestly, if having some additional types will improve the ergonomics for contributors using PyCharm, and it won't become an active land mine for everyone else, then I think I can live without the testing aspect.

However, if you want to catch up to talk through your plans about typing, I'm happy to jump on a call - grab me on Discord whenever you see me; or, if you want to document them on a ticket, I'm happy to respond there too.

[rmartin16]

I don't use PyCharm, so I'm not experiencing the underlying problem that is motivating this change

Out of curiosity, does VS provide typing for tools in ToolCache? For instance, does VS know what methods (and their args) are available on self.tools.subprocess?

At the end of the day, that's the problem I'd really like to solve here.

the big "ignored" import block in integrations/base.py

Agree; I went in to a lot of rabbit holes today trying to find a way to do these imports with coverage. (Although, while the imports don't have coverage, the code also never actually executes either.) Unfortunately, it proved impossible to get coverage without circular import errors....unless I guess we put all the tools and the ToolCache in the same file....or something.

the architectural oddity of add_tools()

Also agree; I can remove add_tools().

Finally, do you see any value in any of the other "testing" I added? I think that's just limited to test_toolcache_typing which only verifies we made the same typo in both locations ;)

[rmartin16]

I removed ToolCache.add_tools() and updated the type testing to be much more dynamic. Feel free to say its too heavy headed and we're probably better off without it. It further reinforces that the name of the tool in the ToolCache must be the value of Tool.name as well as the name of the module. It's intended to ensure all Tools are annotated and any annotations are accurate insofar as they can be resolved to existing classes.

[freakboy3742]

I think we're getting close; one minor cleanup and one ponderous question inline.

[freakboy3742]

It's not a huge deal, but tools_for_module is a moderately complex method with an answer that shouldn't change; both of the invocations are occurring inside for loops. Is there any reason we shouldn't pre-compute once and store, rather than re-computing every time?

[rmartin16]

I avoided caching mostly for simplicity. Although, tools_for_module() should only be called twice for any particular module.

I can add the functools.lru_cache decorator to tools_for_module() to get caching for "free" though.

[freakboy3742]

That's a bit more complexity than is warranted here. This isn't a big deal; I won't go to the mat over 2 calls to a test function that isn't that complex.

[freakboy3742]

I was almost ready to approve this PR... and then I remembered something.

That opens up a bunch of questions:

1. On what condition will TYPE_CHECKING be true?
2. Is it possible for the user to have TYPE_CHECKING be true and not have a working git install?
3. Is there any meaningful fallback/error recovery to use in this case?

[rmartin16]

I was almost ready to approve this PR... and then I remembered something.

That opens up a bunch of questions:

1. On what condition will TYPE_CHECKING be true?

TYPE_CHECKING probably shouldn't ever be True; even for type checkers like mypy, they should only assume TYPE_CHECKING is the same as True.

2. Is it possible for the user to have TYPE_CHECKING be true and not have a working git install?

It's probably possible if something monkeypatched typing.TYPE_CHECKING at interpreter startup. But that definitely isn't the intended way to "assume" TYPE_CHECKING is True when relevant. I really hope the stronger promise of the design of TYPE_CHECKING is that it is always False at runtime.

3. Is there any meaningful fallback/error recovery to use in this case?

Not currently. But if we wanted to ensure this was avoided, we could change if TYPE_CHECKING: to simply if False:. Although, for instance, pytest explicitly moved from if False: once Python 3.5 could use typing.TYPE_CHECKING.

[freakboy3742]

If someone is monkey patching an internal variable like TYPE_CHECKING, I think we're firmly in "you get to keep all the shiny pieces" territory.

On that basis, I think I'm happy with this; if we ever enable mypy as part of the CI checks, we might need to revisit that, but I'm happy to call that a problem for Future Apiarists.