[uss_qualifier/reports] Typing fixes

[the-glu]

updated ./.basedpyright/baseline.json with 2462 errors (went down by 94)
0 errors, 0 warnings, 0 notes

Mostly assert & ignore, as typing don't understand hasattr, and *Type system (nor has_field_with_value, and narrowing is not smart enough yet to indicate we perform the test)

[BenjaminPelletier]

The problem with the approach is that IDEs don't generally seem to find fields by getattr string-literal usage. So, if someone was looking at the field children in the type of object that element is, they would find an instance of that field's usage with the previous approach, but they wouldn't find an instance of that field's usage with this change.

I'm not strongly against changing between two equally-valid approaches if basedpyright prefers one of them (although that is still a use of limited and valuable dev time that could be applied elsewhere), but we certainly shouldn't let basedpyright cause us to switch from an approach with more advantages to an approach with less advantages.

[BenjaminPelletier]

Unlike the comment below, I think we do want to keep the original here to expose the use of the children attribute to IDEs. So, I think #pyright: ignore is appropriate here instead.

[BenjaminPelletier]

The epoch.type check above ensures epoch.case is defined, so I don't think this redundant check is an improvement. The pattern where an object can have one of multiple child payload types (Case or Events in this case; test_scenario, test_suite, action_generator for TestSuiteAction; etc) recurs in this codebase so it seems like we should have a standard approach. Perhaps the type property accessor (or similar) should never be used to switch between behaviors, and instead the fields should be inspected directly? E.g.:

for epoch in scenario.epochs:
    if not epoch.case:
        continue
    if case_name == ...

[the-glu]

I do agree we have it a lot and we should switch to use the fields directly, but this there is one catch: is it safe, through the code base so assume those fields uniquely define the thing we should do? Or in others words, is only one of (test_scenario, test_suite, action_generator) not None?

[BenjaminPelletier]

is it safe, through the code base so assume those fields uniquely define the thing we should do? Or in others words, is only one of (test_scenario, test_suite, action_generator) not None?

That's the intent in these places -- basically equivalent to oneof. We should certainly clarify that through documentation at least anywhere that's unclear. Actual annotations would be awesome, but I don't know of any way to do that in Python. My documentation pattern would probably look something like:

class Epoch(ImplicitDict):
    # === OneOf: Only one of these properties may be specified ===
    case: Optional[TestedCase] = None
    events: Optional[list[Event]] = None
    # === End OneOf ===

The giveaway that this is the case when looking at existing code is something that implies only one may be set; e.g.:

    @property
    def type(self) -> EpochType:
        if self.case:
            return EpochType.Case
        elif self.events:
            return EpochType.Events
        else:
            raise ValueError("Invalid Epoch did not specify case or events")

[BenjaminPelletier]

Isn't the basedpyright finding legitimate here? It seems like .json can be None and we wouldn't want to attempt parsing in that case.

[the-glu]

He is, but I wanted to keep the behavior of raising and exception that is catched by the try/except block. Should we switch to better error handerling there?

[BenjaminPelletier]

It seems like that is relying on a behavior (implementation detail) of ImplicitDict that isn't in the public interface. The typing contract/interface says the first argument needs to be a dict rather than a dict | None or a dict | Any and I don't think we should rely on all implementations double-checking inputs for type-correctness because that's what we have basedpyright for (so we don't need to check and raise on every argument for every function). In this case specifically, ImplicitDict could change its behavior when an invalid argument is passed to raise a custom/different exception type rather than a ValueError and then this usage would break. So yes, I think if we want to trigger the except block when event.query.request.json isn't a dict, we should have explicit logic to do so.

[BenjaminPelletier]

Could you explain basedpyright's complaint here? I don't think we want to blindly ignore findings, so it might be a good idea to always include some kind of justification along with the directive (e.g., for line 160 above and 181+189+194 below, something pointing to the variable argument support of the QueryKMLRenderer protocol despite strict implications of the Protocol).

[BenjaminPelletier]

Per above¹, I don't think we need to capture the justification in the repo any more (so example note on 160, 181, 189, 194 isn't valid). But I don't understand the concern we would be ignoring here. render_info is a value from _query_kml_renderers which is typed as a not-None QueryKMLRenderInfo, and the renderer property is a not-None QueryKMLRenderer, so I don't see how there could be an attribute access issue on this line.

¹In the current phase, we want to add #pyright: ignore to all places where there is nothing we want to change based on basedpyright's finding