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

Improve types for client #2032

Merged
merged 6 commits into from
Feb 20, 2024
Merged

Improve types for client #2032

merged 6 commits into from
Feb 20, 2024

Conversation

alexrudd2
Copy link
Collaborator

Solve another 9 errors with mypy --check-untyped-defs

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.

Looks ok, with one small change for you, and a line I need to check before approving

pymodbus/client/base.py Show resolved Hide resolved
pymodbus/client/serial.py Show resolved Hide resolved
@alexrudd2
Copy link
Collaborator Author

alexrudd2 commented Feb 20, 2024

I do wonder why it does not complain on line 225-229, it is exactly the same issue (recv returns bytes)...that just goes to show how (sorry for the expression) how incompetent and dangerous these checkers can be :-)

Actually, the situation is more complicated :)

def recv(self, size):
"""Receive data.
:meta private:
"""
return size
does not seem correct.

If I keep adding type hints (mostly bytes), the type checkers eventually find this:

def _in_waiting(self):
"""Return waiting bytes."""
return getattr(self.socket, "in_waiting") if hasattr(self.socket, "in_waiting") else getattr(self.socket, "inWaiting")()

Quite similar to #2031, but I do not see the full picture yet. Hence not (yet) annotating recv().

janiversen
janiversen previously approved these changes Feb 20, 2024
@janiversen janiversen dismissed their stale review February 20, 2024 19:38

new findings.

@janiversen
Copy link
Collaborator

The recv() is an old relict, which I thought was removed long time ago....the very old code v2x had that for testing reasons I think.

@janiversen
Copy link
Collaborator

The _in_waiting() looks OK to me, but it the linters do not agree......

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.

You decide when you want to merge this and make more in a new PR

@alexrudd2 alexrudd2 merged commit 5e76261 into dev Feb 20, 2024
1 check passed
@alexrudd2 alexrudd2 deleted the client-types branch February 20, 2024 22:36
@alexrudd2
Copy link
Collaborator Author

Down to 58!

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Mar 2, 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.

None yet

2 participants