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

Use pydantic instead of basic_type_check #917

Open
yoelbassin opened this issue Apr 5, 2024 · 7 comments
Open

Use pydantic instead of basic_type_check #917

yoelbassin opened this issue Apr 5, 2024 · 7 comments

Comments

@yoelbassin
Copy link

yoelbassin commented Apr 5, 2024

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 in capability.py, which all the capabilities inherit from, there is a basic_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 using dataclasses 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 :)

@yoelbassin
Copy link
Author

yoelbassin commented Apr 5, 2024

@sea-bass would love to hear your opinion about this one before I start refactoring stuff locally :)
BTW are there any other active maintainers?

@sea-bass
Copy link
Contributor

sea-bass commented Apr 5, 2024

@EzraBrooks is also active, and I can almost guarantee he and I would both be very much pro type hints! See b434598 for example.

@EzraBrooks
Copy link

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.

@yoelbassin
Copy link
Author

especially where there's special cases for certain types, which exist in a few places

Could you mention some of the places you've seen this, so I would skim through them? :)

@yoelbassin
Copy link
Author

yoelbassin commented Apr 6, 2024

Well, after playing with it a little I found the pydantic version that rosdep supports is at most 1.10 which is a little outdated. The newest pip version is 2.6 where breaking changes were introduced since the 1.* versions.

I'm not sure what approach I should take, either implementing a simpler version of pydantic on dataclass (which really isn't ideal), or using the older pydantic version.

What do you think would be the better approach?

@EzraBrooks
Copy link

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.

# Special case for uint8[], we encode the string

@EzraBrooks
Copy link

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.

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

No branches or pull requests

3 participants