Make client.py typecheck under pyright

[dandavison]

No description provided.

[Sushisource]

Dope, thanks. Might be worth opening a ticket to see if we can somehow make the multi-arg stuff type properly.

[Sushisource]

huh, weird, you think Required would be the default...

[dandavison]

Yes, there's a fair amount of reading material on the topic for anyone so-inclined.
https://peps.python.org/pep-0655/
https://typing.readthedocs.io/en/latest/spec/typeddict.html#required-notrequired

[cretz]

Hrmm, I am trying to remember why I did total=False here. Technically all of these config values are optional if you're using it as a typesafe way to provide parameters to Client.__init__. I wonder if I expected someone to do something like:

extra: client.ClientConfig = { "interceptors": [MyInterceptor()] }

# ...

new_client = Client(my_service_client, **extra)

This PR would break that use case. Also it means that if we ever add fields to __init__ we cannot make them required here if we want to be compatible with people that may be using this type. If we are ok breaking this use case, then I wonder if we should just remove total=False. I can't think of a reason that total=False should be there if every field is Required, can anyone else? Or if we want to allow these to be optional, no need for these new annotations.

[dandavison]

Hm, I'm not sure I should have merged without resolving this conversation. May need to revisit this. E.g. in features repo we attempt to do

register_feature(
    workflows=[Workflow],
    check_result=check_result,
    start=start,
    additional_client_config=ClientConfig(interceptors=[MyClientInterceptor()]),
)

https://github.com/temporalio/features/blob/bump-sdk-version/bin/main/update/client_interceptor/feature.py#L66

which isn't passing type checks.

[cretz]

Hrmm, I wonder if this is the same thing we ignore in the system-agnostic way below and pyright just cares about the line the declaration started instead of the param.

[dandavison]

I think that's right. It's a bit silly having a cluster of type-ignore annotations for different type-checkers but I played around with a few variations and this is what passed.

[cretz]

I am curious, what is the issue on this one? There is ValueType = typing.NewType("ValueType", builtins.int) on the proto side.

[dandavison]

Good call drawing attention to that. It didn't make sense as a pyright false positive to an extent that it needed looking into. The pyright: ignores aren't the right solution; looks like the dataclass decorator is accidental.