Typed ops.pebble.py

[PietroPasotti]

Added types to pebble.py

• several minor API changes (List[str] --> Iterable[str], adding conversions and adapting error messages where necessary)
• small changes to model.py to remove the # type: ignore hanging around the pebble calls

Checklist

• Have any types changed? If so, have the type annotations been updated?
• [NA] If this code exposes model data, does it do so thoughtfully, in a way that aids understanding?
• [NA] Do error messages, if any, present charm authors or operators with enough information to solve the problem?

QA steps

tox -e static

Changelog

• typed ops/pebble.py
• integrated the new types with ops/model.py (previously types were ignored when pebble calls were involved)

[PietroPasotti]

@benhoyt I noticed some inconsistencies in how Client methods check (or don't) inputs.
Many methods used to be typed List[str], are now Iterable[str].
Some methods raise TypeErrors if the input value is not iterable (and/or if the generated values aren't strings), but some others do not. This feels like a good time to consider adding a little utility _try_cast_list_of_strings(obj, allow_none=False) that raises uniformly for all the api methods where Iterable[str] is required? Do you agree?

[PietroPasotti]

@benhoyt I noticed some inconsistencies in how Client methods check (or don't) inputs. Many methods used to be typed List[str], are now Iterable[str]. Some methods raise TypeErrors if the input value is not iterable (and/or if the generated values aren't strings), but some others do not. This feels like a good time to consider adding a little utility _try_cast_list_of_strings(obj, allow_none=False) that raises uniformly for all the api methods where Iterable[str] is required? Do you agree?

Also, I swapped around some arguments to make it more transparent that socket_path:str is a required argument throughout, because the type hints were inconsistent and I thought that eventually an error would be raised if it was None. Can you please double check that?

[pengale]

Overall, looks good. Had one minor comment.

I like how many "fixmes" are going away :-)

[benhoyt]

Thanks for your work on this. I've left a lot of comments. Overall I'm happy to move in the direction of checking the type annotations -- we're using (some) type annotations, so I agree we should check them otherwise they'll get stale. And as you've noted, some or inconsistent already (list vs iterable).

That said, I think this PR as it stands is too invasive and complex for what we need. We're primarily concerned about "typing" the public API, right? In which case I'd much prefer to limit this to add types to just the public parts. I also don't think we should refactor, add helper functions, and so on in this PR -- if needed we can refactor in subsequent PRs. I'm concerned the complexity of this change will introduce subtle changes (I've noted a couple in my comments).

So I think we should reduce the changes here and limit the scope of this PR to just typing (the public parts of the API). IMO we should get it type checking in a fairly minimal and non-invasive way, then if we want to add more typing or do further refactoring, we can do that later in other PRs.

[benhoyt]

This is significantly lighter and less invasive now, thanks! I'd probably still prefer the complicated internal I/O stuff to be untyped, but that's more of a preference.

Yeah, Python text vs bytes I/O is a pain in the neck. I remember writing this same API in the Go client versus this Python one, and the Go io.Reader and io.Writer abstractions (and Go's str being essentially equivalent to Python's bytes) made this soooo much easier in Go. In Python it's so hard to tell what a given "file-like object" supports, whether it's a reader or writer, when it returns str vs bytes, and so on.

I don't think it's so much that it requires refactoring, but just that Python's I/O is kinda messy and vague. I pored over the documentation at the time, but still had to trial-and-error a lot of stuff.

In any case, thanks for your efforts on this, and for the updates.

[rwcarlsen]

Looks pretty good. Just some mostly minor suggestions below.

[rwcarlsen]

I agree that we should probably move the specific case types (e.g. from_dict ones) next to where they are used. Repeating the if TYPE_CHECKING is okay.

[rwcarlsen]

This is a bit confusing. pyright is too dumb to figure out that deadline and timeout are both either None or not None together. But this also basically means that deadline could just have if timeout is not None else 0 and then we don't need any type annotations anywhere.

[PietroPasotti]

True, true, again I tried to refrain from introducing innocent-seeming refactors as part of this PR, fearing to add subtle bugs. In this specific case it does look very innocent, so... @benhoyt can you help confirm?

[PietroPasotti]

@rwcarlsen RE: moving the from_dict types closer to where they are used: I'd like that too, but the problem is that there is some cross-referencing going on (even recursive types, possibly). So I'd rather have them all in one place to make that easier.

I think in the future we could think about moving all of the types to stub files, to keep the source clean.