Skip to content

Improve readability when printing type of classes in error messages #887

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
Open
wants to merge 5 commits into
base: rolling
Choose a base branch
from
Open

Improve readability when printing type of classes in error messages #887

wants to merge 5 commits into from

Conversation

ijnek
Copy link
Contributor

@ijnek ijnek commented Jun 16, 2025

Description

In many places, error messages printed types in a verbose format like '<class 'str'>'. This PR simplifies that by displaying just the type name (e.g. 'str').

For example, given this invalid XML:

<?xml version="1.0"?>
<launch>
  <node />
</launch>

The previous error output was:

- AttributeError: Attribute 'exec' of type '<class 'str'>' not found in Entity 'node'

With this change, it becomes:

- AttributeError: Attribute 'exec' of type 'str' not found in Entity 'node'

Did you use Generative AI?

No

@ijnek ijnek mentioned this pull request Jun 16, 2025
Merged
fujitatomoya
fujitatomoya approved these changes Jun 16, 2025
View reviewed changes
Copy link
Contributor

@fujitatomoya fujitatomoya left a comment

Choose a reason for hiding this comment

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

i think this is more user-friendly error message, lgtm with green CI.

@fujitatomoya
Copy link
Contributor

Pulls: #887
Gist: https://gist.githubusercontent.com/fujitatomoya/67fc4c3c4a310ad34e68e81555fc9f66/raw/8fe810c7c70d60d3809b8a34e1e046b731a4255a/ros2.repos
BUILD args: --packages-above-and-dependencies launch launch_testing launch_yaml launch_xml
TEST args: --packages-above launch launch_testing launch_yaml launch_xml
ROS Distro: rolling
Job: ci_launcher
ci_launcher ran: https://ci.ros2.org/job/ci_launcher/16219

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

christophebedard
christophebedard reviewed Jun 16, 2025
View reviewed changes
@ahcorde
Copy link
Contributor

ahcorde commented Jun 17, 2025 •
edited
Loading

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

This was referenced Jun 17, 2025
Merged
Merged
Closed
christophebedard
christophebedard reviewed Jun 17, 2025
View reviewed changes
Copy link
Member

@christophebedard christophebedard left a comment

Choose a reason for hiding this comment

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

flake8 is complaining about double quotes: https://build.ros2.org/job/Rpr__launch__ubuntu_noble_amd64/284/testReport/junit/launch.test/test_flake8/test_flake8/

  ./launch/utilities/type_utils.py:317:69: Q000 Double quotes found but single quotes preferred
  ./launch/utilities/type_utils.py:463:66: Q000 Double quotes found but single quotes preferred

Also, I think ci.ros2.org is broken for unrelated reasons: ros2/ros2#1699

ahcorde and others added 2 commits June 18, 2025 15:10
@ahcorde @christophebedard
Update launch/launch/utilities/type_utils.py
8e9d31a 
Co-authored-by: Christophe Bedard <[email protected]>
Signed-off-by: Alejandro Hernández Cordero <[email protected]>
@ahcorde @christophebedard
Update launch/launch/utilities/type_utils.py
93a9a94 
Co-authored-by: Christophe Bedard <[email protected]>
Signed-off-by: Alejandro Hernández Cordero <[email protected]>
@ahcorde
Copy link
Contributor

ahcorde commented Jun 18, 2025

Pulls: #887
Gist: https://gist.githubusercontent.com/ahcorde/b622eab172b02dc1d71d4af6d7ac919f/raw/fe9a0f65ec0db0411bf7bad0526f314b88409961/ros2.repos
BUILD args: --packages-above-and-dependencies launch launch_testing launch_xml launch_yaml
TEST args: --packages-above launch launch_testing launch_xml launch_yaml
ROS Distro: rolling
Job: ci_launcher
ci_launcher ran: https://ci.ros2.org/job/ci_launcher/16254

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

christophebedard
christophebedard reviewed Jun 18, 2025
View reviewed changes
Copy link
Member

@christophebedard christophebedard left a comment

Choose a reason for hiding this comment

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

Unfortunately, mypy is still complaining 😅

@ijnek
address pr feedback
35002c9 
Signed-off-by: ijnek <[email protected]>
@ahcorde
Copy link
Contributor

ahcorde commented Jun 19, 2025

Pulls: #887
Gist: https://gist.githubusercontent.com/ahcorde/c98b030f240e9481de3c7ef428aa8122/raw/fe9a0f65ec0db0411bf7bad0526f314b88409961/ros2.repos
BUILD args: --packages-above-and-dependencies launch launch_testing launch_xml launch_yaml
TEST args: --packages-above launch launch_testing launch_xml launch_yaml
ROS Distro: rolling
Job: ci_launcher
ci_launcher ran: https://ci.ros2.org/job/ci_launcher/16265

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

christophebedard
christophebedard reviewed Jun 19, 2025
View reviewed changes
launch_xml/launch_xml/entity.py
attr_error = AttributeError(
"Attribute '{}' of type '{}' not found in Entity '{}'".format(
name, data_type, self.type_name
name, data_type.__name__, self.type_name
Copy link
Member

Choose a reason for hiding this comment

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

it's not reflected in the type annotation above, but data_type can be None apparently here and below :/ see the documentation for this method in the base class:

launch/launch/launch/frontend/entity.py

Line 89 in 6854fae

`data_type = None` will result in yaml parsing of the attribute value as a string.

  1. https://ci.ros2.org/job/ci_linux/24014/testReport/junit/test_launch_ros.test.test_launch_ros.frontend/test_component_container/test_launch_component_container_xml/
  2. https://ci.ros2.org/job/ci_linux/24014/testReport/junit/test_launch_ros.test.test_launch_ros.frontend/test_node_frontend/test_launch_frontend_xml/

I'll open an issue to fix the type annotation.

Copy link
Member

Choose a reason for hiding this comment

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

I'll open an issue to fix the type annotation.

#888

@christophebedard christophebedard mentioned this pull request Jun 19, 2025
Open
@christophebedard
Copy link
Member

And unfortunately RHEL fails for a different reason here and in some other places where the variable can be a typing class like typing.List:

  1. https://ci.ros2.org/job/ci_linux-rhel/3416/testReport/junit/launch_xml.test.launch_xml/test_arg/test_arg_wrong_attribute/
  2. https://ci.ros2.org/job/ci_linux-rhel/3416/testReport/junit/launch.test.launch.utilities/test_type_utils/test_extract_type/
  3. https://ci.ros2.org/job/ci_linux-rhel/3416/testReport/junit/launch.test.launch.utilities/test_type_utils/test_normalize_typed_substitution/

I think it's because it's using an older Python version (3.9) in which typing classes like List don't have a __name__: python/cpython#86495. This was fixed in >=3.10: python/cpython#27237.

So this would require some workaround for those locations

@ahcorde
Copy link
Contributor

ahcorde commented Oct 6, 2025

@ijnek do you mind to take a look ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@christophebedard christophebedard christophebedard left review comments

@fujitatomoya fujitatomoya fujitatomoya approved these changes

Assignees

@ijnek ijnek

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@ijnek @fujitatomoya @ahcorde @christophebedard