-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
[parsing] Improve parser_manual_test #19244
[parsing] Improve parser_manual_test #19244
Conversation
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.
+@jwnimmer-tri for sole review
+(status: single reviewer ok)
+(release notes: none)
Reviewable status: LGTM missing from assignee jwnimmer-tri(platform) (waiting on @jwnimmer-tri)
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.
Reviewed 1 of 1 files at r1, all commit messages.
Reviewable status: 2 unresolved discussions (waiting on @rpoyner-tri)
multibody/parsing/test/parser_manual_test.cc
line 44 at r1 (raw file):
std::vector<ModelInstanceIndex> models; try { models = parser.AddModelsFromUrl(argv[1]);
BTW CI is failing because BUILD.bazel is calling this with a non-URL.
Possibly the tool should recognize URL vs non-URL on the command line (supporting both).
multibody/parsing/test/parser_manual_test.cc
line 46 at r1 (raw file):
models = parser.AddModelsFromUrl(argv[1]); } catch (const std::exception& e) { drake::log()->error(e.what());
I would think we should return 1
to the terminal in this case. If there are errors, we don't want exitcode 0.
These changes push the manual test in the direction of being a useful tool for model checking and parser development. * work more like //tools:model_visualizer * Support ROS_PACKAGE_PATH * Change the input type to allow URLs * Support strict parsing as an option * Improve diagnostics output formatting
26561ec
to
890942b
Compare
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.
Reviewed 1 of 1 files at r2, all commit messages.
Reviewable status: complete! all discussions resolved, LGTM from assignee jwnimmer-tri(platform) (waiting on @rpoyner-tri)
These changes push the manual test in the direction of being a useful tool for model checking and parser development.
This change is