-
Notifications
You must be signed in to change notification settings - Fork 526
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
Use pydantic instead of basic_type_check #917
Comments
@sea-bass would love to hear your opinion about this one before I start refactoring stuff locally :) |
@EzraBrooks is also active, and I can almost guarantee he and I would both be very much pro type hints! See b434598 for example. |
I'd be happy to help review a PR for this. The serialization and deserialization in this package is pretty hard for me to wrap my head around - especially where there's special cases for certain types, which exist in a few places - and it would be great to have static type checking to help trace through it. |
Could you mention some of the places you've seen this, so I would skim through them? :) |
Well, after playing with it a little I found the I'm not sure what approach I should take, either implementing a simpler version of What do you think would be the better approach? |
here's one such odd place, where the library implicitly converts uint8[] to a base64 string. This one in particular actually bit me recently on a project. rosbridge_suite/rosbridge_library/src/rosbridge_library/internal/message_conversion.py Line 203 in 7d78af1
|
It looks like Pydantic is at version 1.10 even on Ubuntu Noble, which releases this month (April 2024), so it seems we should target the older version. |
Currently the
rosbridge_library
doesn't have any type hints (PEP 484). Type hints enhance the readability and maintainability of the codebase.In the
Capability
class incapability.py
, which all the capabilities inherit from, there is abasic_type_check
function which performs type checking for the message fields the capability receives.Instead of using this function, we can use
pydantic
which will 1) perform the type checking and 2) add type annotations for the message fields. If dependencies on pip packages isn't wanted, moving over to usingdataclasses
instead would be a better state than the current list of tuples (which will at least provide some time hints).I think this could improve the codebase quality and would remove redundant code.
I would be happy to work in this PR :)
The text was updated successfully, but these errors were encountered: