-
Notifications
You must be signed in to change notification settings - Fork 566
refactor: Add type annotations to all functions and methods #1069
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
Conversation
It's either none of them or all of them
…nto refactor/add-type-annotations
This fails in Jazzy, due to lack of support for type arguments in Future, Client, Subscription and other classes. @sea-bass Do you think it would be a good idea to branch out for |
I'm OK with branching out! |
This is a massive PR, so I'll be waiting on some more reviews |
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.
I skimmed over it, I don't see any weird stuff. Anything I should pay special focus to? Otherwise I approve.
Probably worth double checking message_conversion module. |
|
||
|
||
def get_encoder(): | ||
def get_encoder() -> Callable[[ListType], bytes]: |
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.
What is the bracket around ListType
doing?
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.
Callable takes 2 type arguments: a list of argument types and the return type. The brackets create a list with a single item
rosbridge_library/src/rosbridge_library/internal/message_conversion.py
Outdated
Show resolved
Hide resolved
rosbridge_library/src/rosbridge_library/internal/message_conversion.py
Outdated
Show resolved
Hide resolved
rosbridge_library/src/rosbridge_library/internal/message_conversion.py
Outdated
Show resolved
Hide resolved
rosbridge_library/src/rosbridge_library/internal/message_conversion.py
Outdated
Show resolved
Hide resolved
rosbridge_library/src/rosbridge_library/internal/message_conversion.py
Outdated
Show resolved
Hide resolved
rosbridge_library/src/rosbridge_library/internal/message_conversion.py
Outdated
Show resolved
Hide resolved
rosbridge_library/src/rosbridge_library/internal/message_conversion.py
Outdated
Show resolved
Hide resolved
rosbridge_library/src/rosbridge_library/internal/message_conversion.py
Outdated
Show resolved
Hide resolved
@MatthijsBurgh How do you propose to modify the type asserts so they raise TypeErrors? |
i.e. # assert isinstance(msg, dict)
if not isinstance(msg, dict):
err_msg = f"msg is not a dict, but a {type(msg)}"
raise TypeError(err_msg) |
@MatthijsBurgh Can you check it now and accept if it looks good to you? |
Public API Changes
Aside from specifying type for public API arguments, the names of any arguments are still thead same.
Description
This was exhausting to do but IMO worth it. It always bothered me that I had to go back and forth between files to figure out what types does rosbridge implementation expect.
Changes
from __future__ import annotations
to every source file - for better compatibility and flexibility in type hinting.required-python
version to 3.10 - so ruff checks don't try to keep compatibility with earlier versionsflake8-annotations
check rules which enforce type hints for every argument and return types in all functions and methods.Additional changes
During the process of adding annotations and fixing type errors, I've made a few implementation changes that might affect the behavior of the library. Most notable changes are:
rosbridge_library.capabilities.advertise.Registration
class, added dummy advertise ID to use in place of None when the client does not provide id of the advertisementrosbridge_library.capabilities.defragmentation
module, got rid ofReceivedFragments
singleton class and instead, addedlists
class variable toDefragment
capabilityrosbridge_library.capabilities.defragmentation.Defragment
class, making use of f-strings where applicablerosbridge_library.internal.message_conversion
module:extract_values
function now calls_from_object_inst
instead of_from_inst
andpopulate_instance
calls_to_object_inst
instead of_to_inst
. We can do this small optimization, assuming theinst
passed to these function is a ROS message type._from_primitive_inst
to split the logic and make it more readable._to_time_inst
a little, raising errors when incorrect type name or message, creating Time and Duration message instances directly, instead of callingrclpy.time.Time().to_msg()
_remove_list_indicators
function