search

RobotWebTools / rosbridge_suite

Server Implementations of the rosbridge v2 Protocol
https://robotwebtools.github.io
BSD 3-Clause "New" or "Revised" License
866 stars 506 forks source link

Use pydantic instead of basic_type_check #917

Open yoelbassin opened 3 months ago

yoelbassin commented 3 months ago

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 commented 3 months ago

@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 commented 3 months ago

@EzraBrooks is also active, and I can almost guarantee he and I would both be very much pro type hints! See https://github.com/RobotWebTools/rosbridge_suite/pull/886/commits/b43459821d74710d46a4f571058571d82d1c4bfe for example.

EzraBrooks commented 3 months ago

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 commented 3 months ago

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 commented 3 months ago

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 commented 3 months ago

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.

https://github.com/RobotWebTools/rosbridge_suite/blob/7d78af16d30d0ffe232abcc65d0928ce90bd61f7/rosbridge_library/src/rosbridge_library/internal/message_conversion.py#L203

EzraBrooks commented 3 months ago

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.