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

also provide --include-hidden-services for ros2 service list verb #551

Merged
merged 3 commits into from
Mar 8, 2022
Merged

also provide --include-hidden-services for ros2 service list verb #551

merged 3 commits into from
Mar 8, 2022

Conversation

Karsten1987
Copy link
Contributor

There's currently API in place for listing hidden services, however the command line option was never exposed.

@Karsten1987 Karsten1987 self-assigned this Jul 7, 2020
@Karsten1987 Karsten1987 added this to Proposed in Foxy Patch Release 1 via automation Jul 7, 2020
@dirk-thomas
Copy link
Member

The argument is already defined on the service command level:

ros2cli/ros2service/ros2service/command/service.py

Lines 24 to 26 in 95bfb05

parser.add_argument(
'--include-hidden-services', action='store_true',
help='Consider hidden services as well')

Otherwise the existing code using args.include_hidden_services would have failed.

@Karsten1987
Copy link
Contributor Author

I see.

It feels weird and inconsistent to me though that ros2 service --include-hidden-services list works while ros2 service list --include-hidden-services fails.
Also ros2 service list -h doesn't mention the option whereas other verbs like find explicitly make use of it.

@Karsten1987 Karsten1987 closed this Jul 7, 2020
@Karsten1987 Karsten1987 deleted the list_hidden_services branch July 7, 2020 19:47
@dirk-thomas
Copy link
Member

whereas other verbs like find explicitly make use of it.

You could add it in the same way as it is done in the find verb:

ros2cli/ros2service/ros2service/verb/find.py

Lines 33 to 36 in 95bfb05

# duplicate the following argument from the command for visibility
parser.add_argument(
'--include-hidden-services', action='store_true',
help='Consider hidden services as well')

@Karsten1987
Copy link
Contributor Author

What's the difference to what was proposed in this PR?

@dirk-thomas
Copy link
Member

dirk-thomas commented Jul 7, 2020 •
edited
Loading

The comment clarifying that this is a known duplication.

@Karsten1987 Karsten1987 restored the list_hidden_services branch July 7, 2020 20:26
@Karsten1987 Karsten1987 reopened this Jul 7, 2020
@Karsten1987
Copy link
Contributor Author

comment added in 5a41e56

@dirk-thomas dirk-thomas changed the title list hidden services also provide --include-hidden-services for ros2 service list verb Jul 7, 2020
@Karsten1987
Copy link
Contributor Author

I'll have to look into the regression introduced from the failing tests below. Don't know right off the bat what's happening there.

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

@jacobperron jacobperron removed this from Proposed in Foxy Patch Release 1 Jul 10, 2020
@jacobperron jacobperron added this to Proposed in Foxy Patch Release 2 via automation Jul 10, 2020
@jacobperron jacobperron removed this from Proposed in Foxy Patch Release 2 Aug 5, 2020
@jacobperron jacobperron added this to Proposed in Foxy Patch Release 3 via automation Aug 5, 2020
@jacobperron jacobperron removed this from Proposed in Foxy Patch Release 3 Oct 9, 2020
@jacobperron jacobperron added this to Proposed in Foxy Patch Release 4 via automation Oct 9, 2020
@jacobperron jacobperron moved this from Proposed to In progress in Foxy Patch Release 4 Oct 9, 2020
@jacobperron jacobperron removed this from In progress in Foxy Patch Release 4 Dec 8, 2020
@homalozoa
Copy link

Friendly ping.

@fujitatomoya
Copy link
Collaborator

@ros-pull-request-builder retest this please

@Karsten1987 @fujitatomoya
list hidden services
da822b5 
Signed-off-by: Karsten Knese <karsten@openrobotics.org>
@Karsten1987 @fujitatomoya
add comment
24aa2c1 
Signed-off-by: Karsten Knese <karsten@openrobotics.org>
@fujitatomoya
Copy link
Collaborator

@ros-pull-request-builder retest this please

@fujitatomoya
Copy link
Collaborator

CI:

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

audrow
audrow approved these changes Mar 8, 2022
View reviewed changes
Copy link
Member

@audrow audrow left a comment

Choose a reason for hiding this comment

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

Looks good with green CI.

@fujitatomoya
change argument order for test_list_hidden.
2a285f5 
Signed-off-by: Tomoya Fujita <Tomoya.Fujita@sony.com>
@fujitatomoya
Copy link
Collaborator

CI(retry):

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

@fujitatomoya fujitatomoya merged commit 478a430 into master Mar 8, 2022
@delete-merged-branch delete-merged-branch bot deleted the list_hidden_services branch March 8, 2022 15:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dirk-thomas dirk-thomas dirk-thomas approved these changes

@audrow audrow audrow approved these changes

@fujitatomoya fujitatomoya fujitatomoya approved these changes

Assignees

@Karsten1987 Karsten1987

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@Karsten1987 @dirk-thomas @homalozoa @fujitatomoya @audrow