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

Don't need to see the full stack trace for ResourceNotFound #1147

Merged
merged 2 commits into from
Aug 22, 2017

Conversation

lucasw
Copy link
Contributor

@lucasw lucasw commented Aug 21, 2017

The user typed in something wrong, or is missing a package, or hasn't sourced the right setup script.

The example that prompted this was using $(find package) where that package wasn't on the path:

<?xml version="1.0"?>
<launch>
  <param name="robot_description" command="$(find xacro)/xacro.py '$(find bad_package)/urdf/foo.xacro'" />
</launch>

The result is a screen full of traceback, followed by a screen full of ROS path output, the line that explains which package wasn't found was sandwiched in-between and is easy to miss.

This eliminates the traceback (but still a lot of ROS path with catkin tools with a workspace with a lot of packages).

Are there other exceptions that are similar that ought to be handled the same?

It would be nice to consolidate the duplicate code in ValueError and RLException above this.

…er typed in something wrong, or is missing a package, or hasn't sourced the setup script.
@dirk-thomas
Copy link
Member

Thanks for the patch.

Are there other exceptions that are similar that ought to be handled the same?

I don't know the answer out of my head. You will need to trace all function calls recursively to find out.

It would be nice to consolidate the duplicate code in ValueError and RLException above this.

Same goes for the newly added case. Do you want to update the PR for that? I don't mind to merge it as-is or with all three blocks merged.

@lucasw
Copy link
Contributor Author

lucasw commented Aug 22, 2017

except(RLException, ValueError, etc) as e: would be cleaner in cfcd0d7, but I like the ability to have custom prefix text to the error output (though all the prefixes are basically the name of the exception type, which could be had from repr + some formatting, but maybe in the future something more verbose would be desired?).

What is a good way to trigger an RLException or a ValueError?
The first thing I tried was <param name="foo" command="" /> which generates an IndexError, which could be added here.

(Is there already a discussion about adding the ability to provide the name and line number of a file where it generates an exception or otherwise a problem is noticed?)

@lucasw
Copy link
Contributor Author

lucasw commented Aug 22, 2017

Generate an RLException:

<param name="foo" command="t" />

(or many other variations where param is spelled wrong, or name, or command)

@dirk-thomas
Copy link
Member

The current patch looks good to me. Do you want to keep iterating on it to cover more cases or should it be merged as is?

Is there already a discussion about adding the ability to provide the name and line number of a file where it generates an exception or otherwise a problem is noticed?

I am not aware of any effort towards such a feature. One problem for this will be that test coverage is very low and the code based not clean and therefore for any change side effects / regressions are highly likely. That was the main reason why other patches to roslaunch haven't been merged in the past.

@lucasw
Copy link
Contributor Author

lucasw commented Aug 22, 2017

Let's merge this, I'll make a separate pr for other cases later, and hopefully also generate some tests for them, but it may take a while.

Also I may not have been clear above (though your answer sounds applicable regardless): the files I want names and line numbers of errors out of are the .launch files.

@dirk-thomas
Copy link
Member

the files I want names and line numbers of errors out of are the .launch files.

I guessed so 😉 Sounds like a nice feature.

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

Successfully merging this pull request may close these issues.

2 participants