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.

Sign up for GitHub

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

Jump to bottom

Replace the use __slots__ for the appropiate API #23

Merged
merged 9 commits into from
Apr 3, 2023
Merged

Replace the use __slots__ for the appropiate API #23

merged 9 commits into from
Apr 3, 2023

Conversation

Voldivh
Copy link
Contributor

@Voldivh Voldivh commented Mar 28, 2023

The current use of the lookup for the fields and field types of a message object is being done by using the attributes __slots__ and __SLOT_TYPES from the object class. This PR uses the correct API get_field_and_field_types() in order to accomplish this.

@Voldivh
Replace the use __slots__ for the appropiate API
38f3694 
Signed-off-by: Voldivh <eloyabmfcv@gmail.com>
@Voldivh Voldivh requested a review from sloretz as a code owner March 28, 2023 17:11
@Voldivh Voldivh mentioned this pull request Mar 28, 2023
Merged
sloretz
sloretz approved these changes Mar 28, 2023
View reviewed changes
Copy link

@sloretz sloretz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM with green CI

clalancette
clalancette requested changes Mar 28, 2023
View reviewed changes
rosidl_runtime_py/convert.py Outdated Show resolved Hide resolved
rosidl_runtime_py/convert.py Outdated Show resolved Hide resolved
@Voldivh
Modifies the comment related to the use of __slots__
74ab83f 
Signed-off-by: Voldivh <eloyabmfcv@gmail.com>
@clalancette
Copy link
Contributor

clalancette commented Mar 28, 2023 •
edited
Loading

CI:

  • Linux Build Status
  • Linux-aarch64 Build Status
  • Windows Build Status

@Voldivh
Adds the '_' character for the lookup of attributes from the message
20b6415 
Signed-off-by: Voldivh <eloyabmfcv@gmail.com>
clalancette
clalancette requested changes Mar 29, 2023
View reviewed changes
Copy link
Contributor

@clalancette clalancette left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch! One fix needed for flake8, then I can run CI again.

rosidl_runtime_py/convert.py Outdated Show resolved Hide resolved
@Voldivh
Modifies the test to use get_fields_and_field_types() method instead …
17d4a83 
…of __slots__

Signed-off-by: Voldivh <eloyabmfcv@gmail.com>
@Voldivh
Adds whitespaces on string definitions
22b1357 
Signed-off-by: Voldivh <eloyabmfcv@gmail.com>
@Voldivh
Removes unnecessary '_' from strings
1282000 
Signed-off-by: Voldivh <eloyabmfcv@gmail.com>
@clalancette
Copy link
Contributor

clalancette commented Mar 29, 2023 •
edited
Loading

OK, this is looking good to me. Let's see what CI has to say about it:

  • Linux Build Status
  • Linux-aarch64 Build Status
  • Windows Build Status

@clalancette
Copy link
Contributor

@Voldivh It looks like there is one more bug with this somehow. See the output from the Linux job at https://ci.ros2.org/job/ci_linux/18361/testReport/junit/ros2topic.ros2topic.test/test_cli/test_cli/:

/verb/echo.py", line 314, in _subscriber_callback
      message_to_yaml(
    File "/home/jenkins-agent/workspace/ci_linux/ws/install/rosidl_runtime_py/lib/python3.10/site-packages/rosidl_runtime_py/convert.py", line 93, in message_to_yaml
      message_to_ordereddict(
    File "/home/jenkins-agent/workspace/ci_linux/ws/install/rosidl_runtime_py/lib/python3.10/site-packages/rosidl_runtime_py/convert.py", line 182, in message_to_ordereddict
      value = _convert_value(
    File "/home/jenkins-agent/workspace/ci_linux/ws/install/rosidl_runtime_py/lib/python3.10/site-packages/rosidl_runtime_py/convert.py", line 212, in _convert_value
      value = __abbreviate_array_info(value, field_type)
    File "/home/jenkins-agent/workspace/ci_linux/ws/install/rosidl_runtime_py/lib/python3.10/site-packages/rosidl_runtime_py/convert.py", line 44, in __abbreviate_array_info
      value_type_name = __get_type_name(field_type.value_type)
  AttributeError: 'str' object has no attribute 'value_type'

Can you take a look?

@Voldivh
Uses the SLOT_TYPES attributes to retrieve the rosidl_paser.definitio…
06724e3 
…n for each message field

Signed-off-by: Voldivh <eloyabmfcv@gmail.com>
@Voldivh
Copy link
Contributor Author

Voldivh commented Mar 30, 2023

The last commit should fix the issue at hand. I think we can trigger CI

clalancette
clalancette requested changes Mar 30, 2023
View reviewed changes
rosidl_runtime_py/convert.py Outdated Show resolved Hide resolved
rosidl_runtime_py/convert.py Outdated Show resolved Hide resolved
@Voldivh
Adds a more explicit comment on what's happening with the message fie…
5e00cd5 
…lds and field types

Signed-off-by: Voldivh <eloyabmfcv@gmail.com>
clalancette
clalancette requested changes Mar 30, 2023
View reviewed changes
Copy link
Contributor

@clalancette clalancette left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just grepping around, it looks like get_message_slot_types also uses __slots__, and so also needs to be fixed in this file.

@Voldivh
Uses the method get_field_and_field_types to retrieve the field names…
b640105 
… of the messages

Signed-off-by: Voldivh <eloyabmfcv@gmail.com>
@clalancette
Copy link
Contributor

All right, let's see what CI has to say about this:

  • Linux Build Status
  • Linux-aarch64 Build Status
  • Windows Build Status

@clalancette clalancette merged commit e2b5b71 into ros2:rolling Apr 3, 2023
@Voldivh Voldivh deleted the voldivh/changes_fields_lookup branch April 3, 2023 14:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@clalancette clalancette clalancette approved these changes

@sloretz sloretz sloretz approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@Voldivh @clalancette @sloretz