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

commands: add get dataset stats #231

Merged
merged 6 commits into from
Oct 14, 2023
Merged

commands: add get dataset stats #231

merged 6 commits into from
Oct 14, 2023

Conversation

joe-prosser
Copy link
Contributor

Adds flag to get stats for datasets

@joe-prosser joe-prosser self-assigned this Oct 9, 2023
@@ -17,7 +17,7 @@ use std::{
str::FromStr,
};

#[derive(Debug, Clone, Deserialize, Serialize, PartialEq, Eq)]
Copy link
Collaborator

Choose a reason for hiding this comment

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

To avoid having to remove the Eq, you should use ordered_float::NotNan<f64> which we already use for this purpose.

@@ -34,6 +34,7 @@ pub struct Dataset {
pub entity_defs: Vec<EntityDef>,
pub label_defs: Vec<LabelDef>,
pub label_groups: Vec<LabelGroup>,
pub num_reviewed: Option<f64>,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do you need something like a #[serde(default)] here, as num_reviewed is not actually returned by the datasets api endpoint?

This is kind of breaking the encapsulation of this type a bit - as it lives in api, I would expect it to map to our api types.

I think it would be better practice to have a struct like

struct DatasetAndStats {
    dataset: Dataset,
    stats: Option<DatasetStats>
}

and you can implement DisplayTable for that instead. Or, just have a type in the cli named Dataset that you can convert From the api Dataset, and add some more stats to.

If you decide not to do the above, I think you need to add a comment documenting that this field is not returned by the datasets api

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep, great point. I'll update

Copy link
Collaborator

@tommilligan tommilligan left a comment

Choose a reason for hiding this comment

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

LGTM, nice!

Comment on lines 107 to 124
impl DisplayTable for DatasetAndStats {
fn to_table_headers() -> Row {
row![bFg => "Name", "ID", "Updated (UTC)", "Title", "Num Reviewed"]
}

fn to_table_row(&self) -> Row {
let full_name = format!(
"{}{}{}",
self.dataset.owner.0.dimmed(),
"/".dimmed(),
self.dataset.name.0
);
row![
full_name,
self.dataset.id.0,
self.dataset.updated_at.format("%Y-%m-%d %H:%M:%S"),
self.dataset.title,
self.stats.num_reviewed
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nice that this is so simple - I was thinking you could delegate through to the Dataset implementation, but that doesn't look simple and this is trivial, so happy with just duplicating it.

@joe-prosser joe-prosser merged commit 2f96ce7 into master Oct 14, 2023
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants