refactor: rely on type annotations instead of casts in hookcmds

[james-garner-canonical]

hookcmds functions that return values generally work by calling json.loads on the result of a hook command CLI invocation. Values returned by json.loads are typed as Any, but we know more about the values returned by different hook commands than this.

Currently, this is expressed by calling typing.cast when calling json.loads. This PR changes this to be expressed with type annotations, for improved readability and lower runtime overhead.

[james-garner-canonical]

This logic is captured by the overloads.

[dimaqq]

Curious, I would have imagined that the cast was there specifically to assuage pyright fears wrt the overloads.

[tonyandrewmeyer]

The cast was added to explicitly document what type we get back from Juju (consistently, across the entire package. Since json.loads returns Any, the awful "ignore all typing" trick, the casting isn't needed if you're willing to trust that the combination of overloads (if any) and primary method signature are enough.

[james-garner-canonical]

Note also that any types declared in the function body don't directly interact with the overload signatures. Pyright only checks that the implementation signature is compatible with the overload signatures, and checks that what it knows about the real return type matches the implementation signature.

[james-garner-canonical]

This logic is captured by the overloads.

[james-garner-canonical]

This logic is captured by the overloads.

[james-garner-canonical]

This logic is captured by the overloads.

[benhoyt]

I like the simplifications, and prefer the type declaration style over the cast() too.

Any reason this should still be a draft PR?

[tonyandrewmeyer]

1. The type narrowing from Any to one specific type can be expressed by a type annotation instead of a cast, which is more idiomatic and has less runtime overhead.

I have no objection to this change.

2. Most of the time when we conditionally narrow the type based on the arguments, we immediately return the result. In this case, the return type annotation already expresses the type narrowing via overloads, so we can directly return json.loads(stdout).

I feel like this goes in the opposite direction from the decision we just made to always have a return type annotation. The argument then was that it's protection against the code and the return value type accidentally diverging, because you get the inferred type and the explicit type. Here, the suggestion is that we go away from the explicit type in the return and explicit casting (or annotating) in the body to only having one of those.

I'm also unsure whether we should be relying on overloads to be the documentation for what Juju returns (having this documentation is the critical point here, in my opinion, because it's not anywhere else other than the Juju code, and it was a lot of work to figure it all out, and we don't want to make it hard for people to work with this in the future). For example, this is a common pattern:

@overload
def tool(key: None = None) -> dict[str, str]: ...
def tool(key: str) -> str: ...
def tool(key: str | None = None) -> str | dict[str, str]:
    ...
    if key is not None:
        result: str = json.loads(stdout)
    else:
        result: dict[str, str] = json.loads(stdout)
    return result

If that is instead:

@overload
def tool(key: None = None) -> dict[str, str]: ...
def tool(key: str) -> str: ...
def tool(key: str | None = None) -> str | dict[str, str]:
    ...
    return json.loads(stdout)

It's shorter and simpler and will pass the type checks, and the type checker will be able to figure out whether the caller is getting a str or dict based on the input. However, as a human reading this code, I have to use the overloads to understand that passing in a key means getting a string back, and otherwise I get a dict. I think that's less clear than the simple if statement, especially if there are actually many combinations of overloads.

@benhoyt asked for all of hookcmds to use the same pattern for loading the content from the hook command, annotating it appropriately, and then returning the value. That's why they all have exactly the same layout even when there's simpler ways of handling some of them. I agree that this consistency has value, particularly if you're adding or editing hookcmds methods. What are we gaining in exchange for losing the consistency?

3. The current pattern involves different runtime code execution paths when the actual difference is at type checking time only

I have some sympathy for this argument. However, it's typically just going to be a cast or annotation, so I don't think there's much risk of, for example, code wrongly diverging. From a coverage point of view, I feel it's actually nicer, since you can tell if there are tests exercising each type, which I don't think you can get otherwise (there's no "typing coverage", right?).

I suspect that this is best discussed in real-time rather than solely in a PR.

[tonyandrewmeyer]

Since this approach was specifically requested in a PR review only a couple of months ago, and since it breaks the consistency that was particularly wanted, and since I'm not convinced that we gain enough from it, or that we are left with enough documentation for humans, I would prefer that this is discussed in real-time before giving this an "approve". Please organise a call (or use the daily Charm Tech sync) for that.

[tonyandrewmeyer]

The cast was added to explicitly document what type we get back from Juju (consistently, across the entire package. Since json.loads returns Any, the awful "ignore all typing" trick, the casting isn't needed if you're willing to trust that the combination of overloads (if any) and primary method signature are enough.

[tonyandrewmeyer]

Thanks!