-
Notifications
You must be signed in to change notification settings - Fork 947
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
Hex values for repl arguments #1075
Conversation
Fix pymodbus-dev#628. int(x, 0) accept decimal (11), hex (0x11=17), as well as binary (ob1100=12) and octo (0o11=9) values.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me, and the documentation support your claim, it would be nice though to see a couple of test cases.
Since @dhoomakethu if the expert on repl, lets hear his opinion.
I added tests for _process_args, which I have to move to module (shall not cause an issue). repl itself is sparsely covered by tests (to be decent), but that shall perhaps be another PR (to add testing for repl client and server). |
Looks good, although I would ideally want to have support only for decimal and hex and not support binary or octal as thats going to cause some confusion or not really used. |
Binary values make sense for various flags encountered in Modbus devices. I would leave it that way, binary support is handy, and octal wouldn't make a big difference. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM.
You are right repl does not have a lot of tests, so feel free to add tests. I am trying to generally cleanup the tests (and the code) but I am stuck with the client/server parts. Feel free to ping me if you want to ping-pong on how to write the tests (I like pytest.mark.parametrice a lot) |
Fix #628. int(x, 0) accept decimal (11), hex (0x11=17), as well as
binary (ob1100=12) and octo (0o11=9) values.