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

Add exception type to error output #753

Merged
merged 3 commits into from
Jan 25, 2024
Merged

Add exception type to error output #753

merged 3 commits into from
Jan 25, 2024

Conversation

dyackzan
Copy link
Contributor

Knowing the type of exception that occurred provides more context and makes launch exceptions easier to track down.

@dyackzan
Add exception type to error output
f38131e 
Knowing the type of exception that occurred provides more context and
makes launch exceptions easier to track down.

Signed-off-by: David Yackzan <dwyackzan@gmail.com>
tylerjw
tylerjw approved these changes Jan 12, 2024
View reviewed changes
Copy link
Contributor

@tylerjw tylerjw left a comment

Choose a reason for hiding this comment

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

This looks like a nice change. Do you have a test for this?

@dyackzan
Copy link
Contributor Author

@tylerjw I just tested locally, but I'll work on getting a test added to this PR

@dyackzan dyackzan force-pushed the add-exception-type-to-error-message-rolling branch from 2cb43ca to 6deaf37 Compare January 12, 2024 17:42
@dyackzan
Add invalid_launch_file_error test
9b67835 
Signed-off-by: David Yackzan <dwyackzan@gmail.com>
@dyackzan dyackzan force-pushed the add-exception-type-to-error-message-rolling branch from 6deaf37 to 9b67835 Compare January 12, 2024 17:52
@dyackzan dyackzan requested a review from tylerjw January 12, 2024 17:56
tylerjw
tylerjw approved these changes Jan 12, 2024
View reviewed changes
Copy link
Contributor

@tylerjw tylerjw left a comment

Choose a reason for hiding this comment

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

Thank you for adding the test. Will it be ok to backport this to Humble and Iron?

@dyackzan @tylerjw
Update Copyright year
d83cc7e 
Co-authored-by: Tyler Weaver <maybe@tylerjw.dev>
Signed-off-by: David Yackzan <dwyackzan@gmail.com>
@dyackzan
Copy link
Contributor Author

@tylerjw I don't think a backport will work here...It looks like the functionality to report multiple exceptions was added into rolling and not backported to humble and iron (from this PR). So this change won't be 1-to-1 and the test I added for the multiple exception case wouldn't pass.
What's the recommended course of action here? Attempt to backport those changes first and then these? Or should I just open separate PRs

@dyackzan dyackzan requested a review from tylerjw January 12, 2024 18:50
@clalancette
Copy link
Contributor

What's the recommended course of action here? Attempt to backport those changes first and then these? Or should I just open separate PRs

Generally, we get the fixes into rolling first, and then open backports. So even though this is going to have to be a "manual" backport (rather than using the bot), we should get this landed first.

@clalancette
Copy link
Contributor

clalancette commented Jan 24, 2024 •
edited
Loading

CI:

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

@dyackzan
Copy link
Contributor Author

CI:

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

@clalancette in the last section of the logs it looks like the test_xmllint.py script failed. I see

14:07:00 test/test_xmllint.py .                                                   [ 38%]
14:07:03 FATAL: command execution failed

but I don't think this would be caused by this PR since I didn't commit any xml changes. Let me know what you think

@clalancette
Copy link
Contributor

but I don't think this would be caused by this PR since I didn't commit any xml changes. Let me know what you think

Yeah, that was just an infrastructure problem. I've kicked off the job again.

@clalancette clalancette merged commit 57f3d3b into ros2:rolling Jan 25, 2024
3 checks passed
dyackzan added a commit to dyackzan/launch that referenced this pull request Jan 25, 2024
@dyackzan
Add exception type to error output
31a5088 
Backport of ros2#753:
* Output exception type for InvalidLaunchFileErrors
* Add test checking for exception type in output

Signed-off-by: David Yackzan <dwyackzan@gmail.com>
@dyackzan dyackzan mentioned this pull request Jan 25, 2024
Merged
clalancette pushed a commit that referenced this pull request Feb 3, 2024
@dyackzan
Backport error message improvements (#754)
266c2b9 
* 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
Reviewers

@adityapande-1995 adityapande-1995 Awaiting requested review from adityapande-1995 adityapande-1995 is a code owner

@methylDragon methylDragon Awaiting requested review from methylDragon methylDragon is a code owner

@wjwwood wjwwood Awaiting requested review from wjwwood wjwwood is a code owner

@tylerjw tylerjw Awaiting requested review from tylerjw

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
@dyackzan @clalancette @tylerjw