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.

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

Improve launch file parsing error messages #626

Merged
merged 4 commits into from Jul 5, 2023

Conversation

timonegk
Copy link
Contributor

This pull request improves the parsing error messages for launch files in two ways:

  1. Parser errors clearly show that values are substituted and not part of the error message itself. I was recently confused by a message Attribute value of type <class 'str'> not found in Entity let, not realizing that the attribute "value" was missing and not some kind of "attribute value".
  2. On an InvalidLaunchFileError, all parsing errors are logged.

Copy link
Member

@audrow audrow left a comment

Choose a reason for hiding this comment

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

One small comment, but otherwise this looks good to me with green CI.

@@ -91,7 +91,7 @@ def get_attr(
If coercion fails, `ValueError` will be raised.
"""
attr_error = AttributeError(
'Attribute {} of type {} not found in Entity {}'.format(
'Attribute `{}` of type `{}` not found in Entity `{}`'.format(
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
'Attribute `{}` of type `{}` not found in Entity `{}`'.format(
"Attribute '{}' of type '{}' not found in Entity '{}'".format(

I would prefer using single quotes (') instead of ticks, like here:

"{}(matcher='{}', handler='{}', handle_once={})".format(
.

The same goes for the other locations.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No problem, I also prefer them. I just did it like the other error messages in the same file.

Copy link
Member

Choose a reason for hiding this comment

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

Oh, you're right. I believe when I made the comment I looked elsewhere in the repository and saw the single quotes in use. It'd be nice to have this more consistent within and across files.

@audrow audrow self-assigned this Jul 26, 2022
@audrow
Copy link
Member

audrow commented Aug 17, 2022

@ros-pull-request-builder retest this please

@audrow
Copy link
Member

audrow commented Aug 17, 2022

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

@timonegk
Copy link
Contributor Author

timonegk commented Mar 1, 2023

@audrow friendly ping

@timonegk
Copy link
Contributor Author

timonegk commented Jul 4, 2023

@audrow ping

@clalancette
Copy link
Contributor

CI:

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

timonegk and others added 3 commits July 4, 2023 18:57
Signed-off-by: Timon Engelke <timon.engelke@online.de>
Signed-off-by: Timon Engelke <timon.engelke@online.de>
Co-authored-by: Chris Lalancette <clalancette@gmail.com>
Signed-off-by: Timon Engelke <timon.engelke@online.de>
@clalancette
Copy link
Contributor

clalancette commented Jul 4, 2023

I rebased this onto the latest. Here is another CI run:

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

Signed-off-by: Chris Lalancette <clalancette@gmail.com>
@clalancette
Copy link
Contributor

clalancette commented Jul 5, 2023

CI again after flake8 fix:

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

@clalancette clalancette merged commit f70f901 into ros2:rolling Jul 5, 2023
2 of 3 checks passed
@timonegk timonegk deleted the improve_error_messages branch July 6, 2023 06:35
wjwwood pushed a commit that referenced this pull request Sep 20, 2023
* Improve launch file parsing error messages

Signed-off-by: Timon Engelke <timon.engelke@online.de>
Co-authored-by: Chris Lalancette <clalancette@gmail.com>
dyackzan added a commit to dyackzan/launch that referenced this pull request Jan 25, 2024
Backport of ros2#626:
* Parser errors clearly show that values are substituted and not part of
  the error message itself.
* On an InvalidLaunchFileError, all parsing errors are logged.

Signed-off-by: David Yackzan <dwyackzan@gmail.com>
clalancette pushed a commit that referenced this pull request Feb 3, 2024
* Improve launch file parsing error messages

Backport of #626:
* Parser errors clearly show that values are substituted and not part of
  the error message itself.
* On an InvalidLaunchFileError, all parsing errors are logged.

* Add exception type to error output

Backport of #753:
* Output exception type for InvalidLaunchFileErrors
* Add test checking for exception type in output

Signed-off-by: David Yackzan <dwyackzan@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants