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

fix: typing hints #10

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

rpoisel
Copy link

@rpoisel rpoisel commented Aug 3, 2023

First of all: thanks for this package. It really saved me some time. 👍

I fixed the typing hints according to the error messages displayed by pyright.

If you want, we could also change all set_* and get_* methods to @property and @property.setter to make it a bit more pythonic. But this, I would do in a separate PR.

@ShayBox
Copy link
Owner

ShayBox commented Aug 3, 2023

The typing hints are useful, but I see no reason to have separate single and multiple methods. It's a breaking change to replace an overloaded parameter that's only used within the update method, and assert the response length matches the request length. As far as I'm aware the modbus library should already error at register bounds or fill in missing data

It's been awhile since I made this, but I don't believe getters and setter properties support parameters to overload the cache state from update.

@rpoisel
Copy link
Author

rpoisel commented Aug 3, 2023

The typing hints are useful, but I see no reason to have separate single and multiple methods. It's a breaking change to replace an overloaded parameter that's only used within the update method, and assert the response length matches the request length. As far as I'm aware the modbus library should already error at register bounds or fill in missing data

Okay, well, then I will make this 100% interface compatible and distinguish between the types inside the read/write method's implementation if necessary.

It's been awhile since I made this, but I don't believe getters and setter properties support parameters to overload the cache state from update.

The idea is to replace only the get_/set_ methods with getters/setters properties. This is more syntactic sugar than an actual change in the implementation. The update() method would stay the same and any caching could be preserved. However, there is not much to win here, so I'll leave that as it is.

@rpoisel rpoisel force-pushed the dev-fix-typing-hints branch from bb77b51 to f45ee21 Compare August 3, 2023 19:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants