-
Notifications
You must be signed in to change notification settings - Fork 164
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
better error message when passing an invalid value to ros2 topic pub #48
Conversation
what message are you testing with? I don't know that this output is immediately useful?
|
I only tried a I will put it back in progress for now. It can be updated after Beta 3. |
If we iterate on this after beta3 maybe working on providing autocomplete ala ros1 for the message in yaml would workaround this issue while providing better user experience |
da0f481
to
1c09563
Compare
I updated this for now to just give a reasonable error message. An example of a valid value and/or completion can be done in the future to further improve the user experience. Back in review. |
@@ -66,6 +66,8 @@ def publisher(message_type, topic_name, values): | |||
module = importlib.import_module(package_name + '.msg') | |||
msg_module = getattr(module, message_name) | |||
values_dictionary = yaml.load(values) | |||
if not isinstance(values_dictionary, dict): | |||
return 'The passed value needs to be a dictionary in YAML format' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
how come it's appropriate to return the error message here instead of raise?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because the return value of this function is also the return value of main
.
A few lines below (line 82) it does the same.
Should the help message be updated to use the dictionary notation ? |
Can you please post the exact command you are invoking. For me the following command (as suggested by the help text) does work fine:
|
Nvm I was missing a space because of the line break in the help... If taking a slightly more complex example: I guess we just need to make sure people don't forget to add a space between each key / value |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
ensure terminating subprocesses on keyboard interrupt
Instead of showing a stacktrace with the following exception:
this patch prints an error message
which includes an example value for the specified message type.