-
Notifications
You must be signed in to change notification settings - Fork 323
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
CLI Command to query connection channels #506
Conversation
Codecov Report
@@ Coverage Diff @@
## master #506 +/- ##
=========================================
+ Coverage 13.6% 29.7% +16.0%
=========================================
Files 69 160 +91
Lines 3752 12917 +9165
Branches 1374 5092 +3718
=========================================
+ Hits 513 3845 +3332
- Misses 2618 8389 +5771
- Partials 621 683 +62
Continue to review full report at Codecov.
|
This should be ready for review. If you wish, @cezarad and @vitorenesduarte please also have a look over the PR and any thoughts or questions would be welcome! |
let vec_ids = response | ||
.channels | ||
.iter() | ||
.filter_map(|ic| ChannelId::from_str(ic.channel_id.as_str()).ok()) |
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.
If there are any errors here, they will be silently ignored. Maybe we can instead do something like this: https://doc.rust-lang.org/rust-by-example/error/iter_result.html#fail-the-entire-operation-with-collect
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.
We can do this but the question is do we want to fail the entire operation if we cannot extract one ChannelId
?
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.
Good catch.
Between the option of failing the operation because an identifier fails to parse, and ignoring the offending identifier(s), I would rather go with the latter.
One middle-road solution is to have warnings in case some identifiers fail to parse. We're not (yet) principled with how we use warnings, if at all, so I'm adding a TODO here to keep an eye out in the future. c51b814
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.
Looks good! Thanks Adi!
* Implemented the 'query connection channels' command * Avoid calls to unwrap * Updated the changelog * Added basic CLI test * Fix FMT * Added JSON output option * Overrriding default tracing cfg [WIP] * Revert "Overrriding default tracing cfg [WIP]" This reverts commit fb2e270. * Simplified output, removed JSON * Fixed output text * Using find_chain method * Added TODO addressing Vitor's comment
Closes: #505
Description
Added the following command:
Sample outputs:
Error-prone cases:
Also offers option for json output (initial experience towards #500):(this work will take place in a different PR)For contributor use:
docs/
) and code comments.Files changed
in the Github PR explorer.