Skip to content

Replace the use __slots__ for the appropiate API#23

Merged
clalancette merged 9 commits intoros2:rollingfrom
Voldivh:voldivh/changes_fields_lookup
Apr 3, 2023
Merged

Replace the use __slots__ for the appropiate API#23
clalancette merged 9 commits intoros2:rollingfrom
Voldivh:voldivh/changes_fields_lookup

Conversation

@Voldivh
Copy link
Copy Markdown
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
Copy Markdown

@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
Comment thread rosidl_runtime_py/convert.py Outdated
Comment thread rosidl_runtime_py/convert.py Outdated
@Voldivh
Modifies the comment related to the use of __slots__
74ab83f 
Signed-off-by: Voldivh <eloyabmfcv@gmail.com>
@clalancette
Copy link
Copy Markdown
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
Copy Markdown
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.

Comment thread rosidl_runtime_py/convert.py Outdated
Voldivh added 2 commits March 29, 2023 12:30
@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 Voldivh requested a review from clalancette March 29, 2023 17:49
clalancette
clalancette reviewed Mar 29, 2023
View reviewed changes
Comment thread rosidl_runtime_py/convert.py Outdated
@Voldivh
Removes unnecessary '_' from strings
1282000 
Signed-off-by: Voldivh <eloyabmfcv@gmail.com>
@clalancette
Copy link
Copy Markdown
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
Copy Markdown
Contributor

clalancette commented Mar 30, 2023

@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
Copy Markdown
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
Comment thread rosidl_runtime_py/convert.py Outdated
Comment thread rosidl_runtime_py/convert.py Outdated
@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
Copy Markdown
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
Copy Markdown
Contributor

clalancette commented Mar 30, 2023

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.

3 participants

@Voldivh @clalancette @sloretz