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

print_supported implemented for recv and send tools. #642

Merged
merged 1 commit into from
Nov 14, 2023

Conversation

Hrick87
Copy link
Contributor

@Hrick87 Hrick87 commented Nov 12, 2023

Issue #578

I am uncertain if the consolidated interface enumeration and its corresponding protocol should be a part of the printed list, but otherwise I believe I've done this all correctly.

@gavv gavv added ready for review Pull request can be reviewed contribution A pull-request by someone else except maintainers labels Nov 12, 2023
Copy link
Member

@gavv gavv left a comment

Choose a reason for hiding this comment

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

Nice, thanks for PR! Here are a few comments.

src/internal_modules/roc_address/print_supported.cpp Outdated Show resolved Hide resolved
src/internal_modules/roc_address/print_supported.cpp Outdated Show resolved Hide resolved
src/internal_modules/roc_address/print_supported.h Outdated Show resolved Hide resolved
src/internal_modules/roc_address/print_supported.h Outdated Show resolved Hide resolved
src/internal_modules/roc_address/protocol_map.h Outdated Show resolved Hide resolved
src/internal_modules/roc_address/print_supported.h Outdated Show resolved Hide resolved
src/internal_modules/roc_address/protocol_map.cpp Outdated Show resolved Hide resolved
src/internal_modules/roc_address/protocol_map.cpp Outdated Show resolved Hide resolved
src/internal_modules/roc_address/print_supported.cpp Outdated Show resolved Hide resolved
@gavv gavv added needs revision Pull request should be revised by its author and removed ready for review Pull request can be reviewed labels Nov 12, 2023
@gavv
Copy link
Member

gavv commented Nov 12, 2023

I am uncertain if the consolidated interface enumeration and its corresponding protocol should be a part of the printed list, but otherwise I believe I've done this all correctly.

Yep, consolidated should be included.

@Hrick87 Hrick87 force-pushed the print_supported_protos branch from 3ff1044 to 3197896 Compare November 13, 2023 07:27
@gavv gavv added ready for review Pull request can be reviewed and removed needs revision Pull request should be revised by its author labels Nov 13, 2023
@gavv gavv force-pushed the print_supported_protos branch from 3197896 to c62062b Compare November 14, 2023 11:11
@gavv gavv merged commit 789489c into roc-streaming:develop Nov 14, 2023
@github-actions github-actions bot removed the ready for review Pull request can be reviewed label Nov 14, 2023
@gavv
Copy link
Member

gavv commented Nov 14, 2023

Thanks for update, LGTM!

BTW, please don't resolve discussions by yourself, keep them for reviewer, otherwise it's hard to track what is needed to be re-reviewed. Instead you can add comment or 👍 on discussion to indicate that it was addressed.

@Hrick87
Copy link
Contributor Author

Hrick87 commented Nov 14, 2023

Thanks for update, LGTM!

BTW, please don't resolve discussions by yourself, keep them for reviewer, otherwise it's hard to track what is needed to be re-reviewed. Instead you can add comment or 👍 on discussion to indicate that it was addressed.

Ah my mistake, understood. I'll stick to using comments/thumbs up instead. I thought the resolve something each of us did individually on our own ends.

@Hrick87 Hrick87 deleted the print_supported_protos branch November 14, 2023 18:42
@gavv gavv added this to the 0.3.0 milestone Nov 17, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
contribution A pull-request by someone else except maintainers
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants