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 ModbusClientMixin Generic to fix type issues for sync and async #1980

Merged
merged 8 commits into from
Feb 9, 2024

Conversation

laundmo
Copy link
Contributor

@laundmo laundmo commented Feb 8, 2024

this way, the concrete client can specify the generic response return type.

closes: #1973

Copy link
Collaborator

@janiversen janiversen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A very elegant PR, I like it !!

I see that CI is failing with mypy complaining, please fix that, along with the couple of review comments I made.

pymodbus/client/base.py Show resolved Hide resolved
pymodbus/client/base.py Show resolved Hide resolved
@janiversen
Copy link
Collaborator

@alexrudd2 Can you please have a look at this PR, from the typing perspective ? I see it as a very elegant solution to the problem at hand, but you know I am by far not the expert on mypy.

pymodbus/client/mixin.py Show resolved Hide resolved
@janiversen
Copy link
Collaborator

Once you feel its ready to go, please re-request review.

@alexrudd2
Copy link
Collaborator

@alexrudd2 Can you please have a look at this PR, from the typing perspective ? I see it as a very elegant solution to the problem at hand, but you know I am by far not the expert on mypy.

Sure, it may take me a few days though

@alexrudd2 alexrudd2 self-requested a review February 9, 2024 05:29
Copy link
Collaborator

@alexrudd2 alexrudd2 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You clearly know more than me about Python types, and this is a good solution to a problem Jan and I wasted a lot of cycles on. Thank you.

@alexrudd2
Copy link
Collaborator

Oh, if I had to guess you didn't find check_ci.sh or the git hook. (That should be described in CONTRIBUTING.md) Also, you may want to consider installing the ruff extension into VSCode.

@janiversen
Copy link
Collaborator

It is in README:

Development Instructions
------------------------
The current code base is compatible with python >= 3.8.

Here are some of the common commands to perform a range of activities::

   source .venv/bin/activate   <-- Activate the virtual environment
   ./check_ci.sh               <-- run the same checks as CI runs on a pull request.

@janiversen
Copy link
Collaborator

Let get this merged...

@laundmo I look forward to see followup PR, you really did magic here !

Copy link
Collaborator

@janiversen janiversen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks a lot.

The open comments are not urgent.

Copy link
Collaborator

@janiversen janiversen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

need to please ruff

pymodbus/client/mixin.py Outdated Show resolved Hide resolved
@janiversen janiversen merged commit 7f469d5 into pymodbus-dev:dev Feb 9, 2024
1 check passed
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Feb 20, 2024
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.

Improve sync and async type checks instead of Union[sync | async]
3 participants