-
Notifications
You must be signed in to change notification settings - Fork 1
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
commands: add source statistics to table output #225
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.
LGTM, two minor comments
cli/src/commands/get/sources.rs
Outdated
sources | ||
.iter() | ||
.map(|source| { | ||
info!("Getting statistics for source {}", source.full_name().0); | ||
let stats = client | ||
.get_source_statistics( | ||
&source.full_name(), | ||
&StatisticsRequestParams { | ||
comment_filter: Default::default(), | ||
}, | ||
) | ||
.expect("Could not get statistics for source"); | ||
(source.id.clone(), stats) | ||
}) | ||
.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.
Should raise error rather than expect
ing here, as per previous reviews:
sources | |
.iter() | |
.map(|source| { | |
info!("Getting statistics for source {}", source.full_name().0); | |
let stats = client | |
.get_source_statistics( | |
&source.full_name(), | |
&StatisticsRequestParams { | |
comment_filter: Default::default(), | |
}, | |
) | |
.expect("Could not get statistics for source"); | |
(source.id.clone(), stats) | |
}) | |
.collect() | |
sources | |
.iter() | |
.map(|source| { | |
info!("Getting statistics for source {}", source.full_name().0); | |
let stats = client | |
.get_source_statistics( | |
&source.full_name(), | |
&StatisticsRequestParams { | |
comment_filter: Default::default(), | |
}, | |
) | |
.context("Could not get statistics for source")?; | |
Ok((source.id.clone(), stats)) | |
}) | |
.collect()? |
or something like this
cli/src/printer.rs
Outdated
if let Some(stats) = &self.stats { | ||
stats.num_comments.to_string() | ||
} else { | ||
"N/A".to_string() |
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.
For consistency with above?
"N/A".to_string() | |
"none".dimmed() |
Optionally print
num_comments
when getting sources