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

Allow List[int] or int but not float or str #1377

Merged
merged 5 commits into from
Feb 24, 2023
Merged

Conversation

alexrudd2
Copy link
Collaborator

See #1363.

This does not fully close that issue, but at least corrects the type hints.

@alexrudd2
Copy link
Collaborator Author

@janiversen
I'm going to leave this here to not interrupt your further work with the mixin/base.

@janiversen janiversen added this to the Version 3.2 milestone Feb 21, 2023
@janiversen
Copy link
Collaborator

@alexrudd2 this one can be merged, when you want, just change it to "ready for review", I solved a couple of issues.

@janiversen
Copy link
Collaborator

While at it, I corrected mixin.py, so it gets a green light from mypy.

@alexrudd2 alexrudd2 marked this pull request as ready for review February 24, 2023 15:20
@alexrudd2
Copy link
Collaborator Author

While at it, I corrected mixin.py, so it gets a green light from mypy.

Ok, good. I tried other options, but the only two that worked were
(1) removing the return type from execute() => leaves the API untyped
(2) broadening the return types to ModbusResponse (which you've done)

@alexrudd2
Copy link
Collaborator Author

Things brings it down from 150 to 5 mypy errors, all of them in base.py i think. 🎉

@janiversen janiversen merged commit 2c2e859 into dev Feb 24, 2023
@janiversen janiversen deleted the no-float-messages branch February 24, 2023 16:19
@janiversen
Copy link
Collaborator

client/base.py is up for a major change, so it is not really worth while investing too much time there.

I know mypy allows a list of files not to consider in the configuration, using that would allow us to add mypy to CI right now, and then you can reduce the list as you go along.

@janiversen
Copy link
Collaborator

And just for the record, the old return scope was far too limited, because there are something like 5 classes that can be returned all included in ModbusResponse.

Actually using ModbusResponse, might have a positive effect, because users see that all calls return the same struct (with a field not being filled depending on the request).

@alexrudd2
Copy link
Collaborator Author

client/base.py is up for a major change, so it is not really worth while investing too much time there.

I know mypy allows a list of files not to consider in the configuration, using that would allow us to add mypy to CI right now, and then you can reduce the list as you go along.

Yes, it's why I've not bothered.

I will see if I can get things ready for CI (and enable py.typed). In fact this week, I already tried running mypy on some of my applications which use mypy as a library and it appeared to work.

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

2 participants