[mock_uss] Set of typing fixes

[the-glu]

updated ./.basedpyright/baseline.json with 2759 errors (went down by 91) :)

[BenjaminPelletier]

Why drop the return type annotation?

[the-glu]

Because kml.Folder is not a type, it's a variable, they seems to use ElementMaker to generate them on the fly.

Not sure how to do better in that case ^^'

[BenjaminPelletier]

It seems like this is an instance where the strictness of the checker might lead us to lower the quality of our codebase to make it happy -- I think we need a clear plan to guard against that happening. pykml seems to generate types at runtime in a way I don't particularly like, but I think we need to be able to deal with unusual approaches like that since we can't afford to write all our own libraries. I can think of a few ways to approach this, though I think probably option 2 is probably the best balance of effort and utility:

1. Just leave errors we shouldn't correct in the baseline until we've resolved all the other errors. This seems like the easiest approach, but has the downside that people later trying to fix errors will not be able to see that the error has already been evaluated by an earlier reviewer.
2. Add explicit per-line/occurence opt-out flags. This seems to be what we're doing in general (noqa) and seems not too hard, but also signals later reviewers unlike option 1.
3. Add something that works technically and contains more information -- like, create our own static Folder typing object that refers to kml and include comments about usage and properties. This seems like too much work :)

[the-glu]

I do agree with you, but I'm not sure in that particular case that the checker is wrong, see that comment on pyright: microsoft/pyright#8129 (comment)

I'm not sure having an invalid annotation ignored is not worse than having no annotation ;)

One alternative would be to type it with the base class (ObjectifiedElement).

And finally, for that specific function, do we really need the 'correctest' type declared? to_kml_folder is internal to kml.py and no exposed anywhere ^^'

Witch solution do you prefer between opt-out, ObjectifiedElement and implicit typing? I will apply it then everywhere for pykml, that not the first one for that lib that the checker doesn't like ;)

[BenjaminPelletier]

Is there a reason to change this? login_required is written so that it works with or without parentheses, and adding them with no parameters seems to just add an extra call.

[the-glu]

basedpyright seems to be confused in the non-function mode:

error: Impossible d’affecter l’argument de type « ((f: Unknown) -> _Wrapped[..., Unknown, ..., Unknown | Response]) | _Wrapped[..., Unknown, ..., Unknown | Response] » au paramètre de type « T_route@route »
    Le type « ((f: Unknown) -> _Wrapped[..., Unknown, ..., Unknown | Response]) | _Wrapped[..., Unknown, ..., Unknown | Response] » n’est pas assignable au type « RouteCallable »
      Le type « ((f: Unknown) -> _Wrapped[..., Unknown, ..., Unknown | Response]) | _Wrapped[..., Unknown, ..., Unknown | Response] » n’est pas assignable au type « RouteCallable »
        Le type « (f: Unknown) -> _Wrapped[..., Unknown, ..., Unknown | Response] » n’est pas assignable au type « RouteCallable »
          Le type « (f: Unknown) -> _Wrapped[..., Unknown, ..., Unknown | Response] » n’est pas assignable au type « (...) -> ResponseReturnValue »
            Le type de retour de fonction "_Wrapped[..., Unknown, ..., Unknown | Response]" est incompatible avec le type "ResponseReturnValue"
          Le type « (f: Unknown) -> _Wrapped[..., Unknown, ..., Unknown | Response] » n’est pas assignable au type « (...) -> Awaitable[ResponseReturnValue] »
            Le type de retour de fonction "_Wrapped[..., Unknown, ..., Unknown | Response]" est incompatible avec le type "Awaitable[ResponseReturnValue]" (reportArgumentType)

And I think (but that my opinion ^^') that it look cleaner to always have decorator in one consistent mode instead, meaning we can also write it more cleaning. I'm not sure we gain much by avoid one call (and it's not even sure it's avoided ;) )

[BenjaminPelletier]

It seems like some decorators are written to have no parentheses, some are written to have parentheses, and some are written so either is fine. So, I'm not sure how we can necessarily be consistent if we sometimes use decorators from libraries. It worries me a little that the checker seems to falsely flag this as incorrect, but parentheses certainly aren't wrong or worse, so perhaps it's not worth pursuing this particular checker failure further.

[the-glu]

So, I'm not sure how we can necessarily be consistent if we sometimes use decorators from libraries

Yes, I was thinking about consistently for a single decorator, not across them ^^'

[BenjaminPelletier]

The purpose of having a more qualified reference here was to make it clearer what was being called (we're going to fetch RID ISAs). If we remove the qualification, the appropriate function name would probably be fetch_rid_isas or perhaps fetch_isas. Is there a reason to remove the qualification?

[the-glu]

I switched them to a more appropriate name, the core reason is that they are not explicitly exposed in __inits__, so the parsed it not happy :)

We can also update exposed module if you prefer :)

[BenjaminPelletier]

Shouldn't we raise here? If we return -1, is that ever valid and desired?

[the-glu]

I switched, I tried to be as 'no-crashing' as possible, because here it's quite strange: there is a check on line 310, but the part that use dt is not contained in the if. Is that an error and should we just condition the whole name creation bellow ?

[BenjaminPelletier]

I think we should certainly avoid crashing whenever practical. But, any time something we don't intend to happen happens, further behavior is not necessarily fully defined and it's much better to crash (fail fast) at that point rather than continue operation which may lead to a crash in a misleading place or mysterious behavior that will be difficult to track down to its source.

I think probably the idea is that reference_time will always be populated when needed, but I agree that we shouldn't trust that assumption. Something like dt(v.volume.time_start) if v.volume.time_start and self.reference_time else '?' seems appropriate.

[the-glu]

I switched to no adding the timing suffix in that case and kept only the name that seems a better fallback than having {name} ?s-?s ^^'