-
Notifications
You must be signed in to change notification settings - Fork 221
Show IDL interfaces in their original format #1256
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
base: rolling
Are you sure you want to change the base?
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,48 @@ | ||
| # Copyright 2026 Old-Ding | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this is just a test for API, i think unit test for actually idl types should be added to ros2cli_test_interfaces.
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thank you. I replaced the temporary API-only test in 1aea823 with real interfaces in
These are CLI integration tests, rather than a mocked
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Update with the requested ROS test result: https://github.com/Old-Ding/ros2cli/actions/runs/29411539491 I built the current PR head in an Ubuntu ROS Rolling container and ran the |
||
| # | ||
| # Licensed under the Apache License, Version 2.0 (the "License"); | ||
| # you may not use this file except in compliance with the License. | ||
| # You may obtain a copy of the License at | ||
| # | ||
| # http://www.apache.org/licenses/LICENSE-2.0 | ||
| # | ||
| # Unless required by applicable law or agreed to in writing, software | ||
| # distributed under the License is distributed on an "AS IS" BASIS, | ||
| # WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
| # See the License for the specific language governing permissions and | ||
| # limitations under the License. | ||
|
|
||
| """Tests for the interface show verb.""" | ||
|
|
||
| from ros2interface.verb import show | ||
|
|
||
|
|
||
| def test_show_msg_interface(tmp_path, monkeypatch, capsys): | ||
| msg_text = 'uint32 value\n' | ||
| msg_path = tmp_path / 'Basic.msg' | ||
| msg_path.write_text(msg_text) | ||
| monkeypatch.setattr(show, 'get_interface_path', lambda _: str(msg_path)) | ||
|
|
||
| show._show_interface('test_interfaces/msg/Basic') | ||
|
|
||
| assert capsys.readouterr().out == msg_text | ||
|
|
||
|
|
||
| def test_show_idl_interface(tmp_path, monkeypatch, capsys): | ||
| idl_text = """\ | ||
| module test_interfaces { | ||
| module msg { | ||
| struct IdlOnly { | ||
| uint32 value; | ||
| }; | ||
| }; | ||
| }; | ||
| """ | ||
| idl_path = tmp_path / 'IdlOnly.idl' | ||
| monkeypatch.setattr(show, 'get_interface_path', lambda _: str(idl_path)) | ||
|
|
||
| for file_text in (idl_text, idl_text.rstrip('\n')): | ||
| idl_path.write_text(file_text) | ||
| show._show_interface('test_interfaces/msg/IdlOnly') | ||
|
|
||
| assert capsys.readouterr().out == idl_text | ||
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.
i think this just displays whatever format the original is in. but it's a UX regression relative to what users get for .msg, where the whole point of interface show is that you see the full recursive structure without opening more files. i think recursive approach or consideration is completely off from this PR.
and what if the case with " .msg parent with an .idl-only nested type"?
the new IDL branch ignores indent_level (and the comment flags) entirely, raw module/struct boilerplate lands un-indented, i think this prints ugly...
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.
Thank you. I agree that an unindented raw IDL block in a nested expansion is poor UX. In 1aea823, the raw-IDL branch now applies the existing
indent_levelrecursively to every line. The new CLI integration test covers a.msgparent with an IDL-only nested type.I kept the IDL body in its original format intentionally: the maintainer guidance in #780 is that
ros2 interface showshould display the original format because some IDL cannot be converted to.msg/.srv(#780 (comment)).That decision means the raw IDL branch currently preserves IDL comments for both comment options. Applying
--no-comments/--all-commentsto raw IDL would require defining an IDL-specific transformation rather than displaying its source. Could you please confirm whether you want that new formatting behavior, or whether preserving raw IDL with correct recursive indentation is the intended scope for this PR?