Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make client type annotations compatible with async client usage #1842

Merged
merged 3 commits into from
Oct 19, 2023

Conversation

jvelo
Copy link
Contributor

@jvelo jvelo commented Oct 19, 2023

@janiversen
Copy link
Collaborator

That is often a fluke, I have restarted it.

@alexrudd2
Copy link
Collaborator

Thanks for trying this; I had gotten stuck earlier. The use of type Awaitable is a good idea.

However, I'm not sure this is the right approach. The key is having the right type signatures below. (They're currently unchecked because they have no types at all). Perhaps you understand how to do it better than my attempts.

https://github.com/pymodbus-dev/pymodbus/blob/dev/pymodbus/client/base.py#L168-L212

@janiversen
Copy link
Collaborator

#@alexrudd2 I highly disagree with you, we will maintain duplicated code just to please mypy, especially not when we have a valid alternative.

You can find the same approach in many standard libraries. This is totally legal and not against any PEP. Remark the function just passed the future it does not create it. The future is created in a method properly marked with async.

@jvelo jvelo changed the title Try and see if CI will accept changes proposed in #1821 Make client type annotations compatible with async client usage Oct 19, 2023
@janiversen janiversen merged commit 01ad131 into pymodbus-dev:dev Oct 19, 2023
1 check passed
@janiversen
Copy link
Collaborator

@jvelo thanks for the PR, to me it an excellent compromise, it does not cause duplicate code, it violates no PEP and it silences mypy.

"async' is demanded for functions that use asyncio functionality.

@jvelo
Copy link
Contributor Author

jvelo commented Oct 19, 2023

There's still one aspect we might have overlooked: verification of synchronous code where previously code that would expect ModbusResponse might not accept the new union type (while for async code it's OK since the await will "unbox" the Awaitable) ...
Not sure how other libraries deal with this, but I can try and look into it

@jvelo
Copy link
Contributor Author

jvelo commented Oct 19, 2023

I confirm that the change I've proposed is not such an improvement over the status quo, since now both sync and async clients cannot make assumption over which side of the union will be returned :/ Sorry to have rushed this, I though it would be simpler than it is.
I tried to play with @overload too but to no avail, it doesn't seem to support overloading with the same signature just one async the other not.
Right now I can't see an elegant alternative that would avoid code duplication. Maybe the better tradeoff would be to have a Protocol for async with the async signatures, so that client code that use an async version can box to this protocol.

@alexrudd2
Copy link
Collaborator

There's still one aspect we might have overlooked: verification of synchronous code where previously code that would expect ModbusResponse might not accept the new union type

Yes, you've articulated the problem I wasn't able to express well :)

@alexrudd2
Copy link
Collaborator

@jvelo thanks for the PR, to me it an excellent compromise, it does not cause duplicate code, it violates no PEP and it silences mypy.

It does not silence mypy. There are still errors if you use --check-untyped-defs. All this PR has done is move the location :)

@janiversen
Copy link
Collaborator

I suspect we end up writing a generator that generates all functions as sync/async, that is a solution acceptable because it still have a low maintenance point.

Basically it could be done with "sed" search for "def" and change to "async def", however the problem is to automate this,

@alexrudd2
Copy link
Collaborator

Ideally it should be possible to write one wrapper (decorator perhaps?) instead of changing all functions. I tried earlier but could not figure it out.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Oct 30, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants