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

feat(cli): release date column for package listings #4203

Open
wants to merge 10 commits into
base: master
Choose a base branch
from

Conversation

EstebanBorai
Copy link
Contributor

@EstebanBorai EstebanBorai commented Oct 2, 2024

Hub Clients such as:

  • cdk
  • fluvio hub connector list (uses same impl as from cdk)
  • fluvio hub smartmodule list

Should include the release date as part of their listing.

Demo

CleanShot.2024-10-03.at.13.00.19.mp4

@EstebanBorai EstebanBorai linked an issue Oct 2, 2024 that may be closed by this pull request
@EstebanBorai EstebanBorai force-pushed the 4185-cdk-list-release-date branch 4 times, most recently from 27b79dd to f72d62a Compare October 2, 2024 22:42
@EstebanBorai EstebanBorai changed the title feat(cdk): hub list should include release date feat(hub): hub lists should include release date Oct 2, 2024
@EstebanBorai EstebanBorai changed the title feat(hub): hub lists should include release date feat(hub): package listings column for release date Oct 2, 2024
@EstebanBorai EstebanBorai changed the title feat(hub): package listings column for release date feat(hub): release date column for package listings Oct 2, 2024
return date.format("%Y-%m-%d %H:%M:%S %Z").to_string();
}

if duration.num_weeks() >= 1 {
Copy link
Contributor

Choose a reason for hiding this comment

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

This ia already available in humantime crate. No need to reinvent

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes that was my first attempt, using humantime, but I didn't found a format specifier,FormatDuration doesn't let you pick which fields to use.

And given that release dates are from more that a year in some cases we have outputs like 2years 2min 12us (as shown here: https://docs.rs/humantime/latest/humantime/)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Here is an example of how many packages looked like: 2years 12h 4m 37s 47ms 997us 519ns

https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=933296e2dc7dcd9f93dc91fc75849617

It would be nice to use from years to minutes but thing is that theres no way to specify that.

Copy link
Contributor

@ajhunyady ajhunyady Oct 3, 2024

Choose a reason for hiding this comment

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

That's too much. It should be the last value, 2 years, or the last 2 values, 4 days 26 min.

Copy link
Contributor

@ajhunyady ajhunyady Oct 3, 2024

Choose a reason for hiding this comment

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

BTW, you can use Kubernetes for hints. The output for accounts has a time value.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thats correct @ajhunyady! Thank you so much for the suggestions!

I have updated tests to use a static past date and use that for calculations, so dynamic current time doest break them.

Yeah it would be nice to have humantime format support but seems that its not being actively maintained, last commit was 2 years ago: https://github.com/tailhook/humantime

use chrono::{DateTime, Utc};

pub fn format_duration(date: DateTime<Utc>, exact: bool) -> String {
let duration = Utc::now().signed_duration_since(date);
Copy link
Contributor

Choose a reason for hiding this comment

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

Lets add unit test

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Tests provided here: 231efc9

@EstebanBorai EstebanBorai changed the title feat(hub): release date column for package listings feat(cli): release date column for package listings Oct 3, 2024
@EstebanBorai EstebanBorai requested a review from sehz October 3, 2024 16:06
@ajhunyady
Copy link
Contributor

@EstebanBorai can we default to --exact-time? The data alone is not sufficient in most cases.

A nice improvement would be to have human readable values 3 days, or 24 minutes, etc.
And maybe default to that.

@EstebanBorai
Copy link
Contributor Author

@EstebanBorai can we default to --exact-time? The data alone is not sufficient in most cases.

A nice improvement would be to have human readable values 3 days, or 24 minutes, etc. And maybe default to that.

I could make it the default and perhaps later implement the duration since

@EstebanBorai
Copy link
Contributor Author

@sehz @ajhunyady I have updated the formatter so we can inject a comparison date to improve testing. Now tests will not break because we use a fixed date.

With that I think we are good to go?

@EstebanBorai
Copy link
Contributor Author

@EstebanBorai can we default to --exact-time? The data alone is not sufficient in most cases.

A nice improvement would be to have human readable values 3 days, or 24 minutes, etc. And maybe default to that.

Yeah thats the default, in the video we just see the date string output due to the fact that these packages are from more than a month ago

@EstebanBorai
Copy link
Contributor Author

@EstebanBorai can we default to --exact-time? The data alone is not sufficient in most cases.
A nice improvement would be to have human readable values 3 days, or 24 minutes, etc. And maybe default to that.

Yeah thats the default, in the video we just see the date string output due to the fact that these packages are from more than a month ago

Here are tested all variants: https://github.com/infinyon/fluvio/pull/4203/files#diff-779c45328cff39a63747a2e73d809fa7c2e7d8fda7e0e88e5a90fb07b0b5af6aR57-R140

@ajhunyady
Copy link
Contributor

Is there a video?

@EstebanBorai
Copy link
Contributor Author

Is there a video?

Yeah! On the top in the PR description Im showing both use cases (exact and not exact) and also for connectors and smart modules.

@@ -81,7 +98,7 @@ mod output {
impl TableOutputHandler for ListSmartModules {
/// table header implementation
fn header(&self) -> Row {
Row::from(["SMARTMODULE", "Visibility"])
Row::from(["SmartModule", "Visibility", "Released"])
Copy link
Contributor

Choose a reason for hiding this comment

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

should be published since there there is no concept of released concept in hub

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sounds good! Applied here: ba6348a

.expect("failed to set min");
let formatted = make_fixed_time_elapsed_fmtr().time_since_str(date, true);

assert_eq!(formatted, date.format(EXACT_FORMAT).to_string());
Copy link
Contributor

Choose a reason for hiding this comment

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

test should have assertion like this:

assert_eq!(date,"December 1989");

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Of course! Here is the update: 0a4d54a

.expect("failed to sub days");
let formatted = make_fixed_time_elapsed_fmtr().time_since_str(date, false);

assert_eq!(formatted, date.format(SIMPLE_DATE_FORMAT).to_string());
Copy link
Contributor

Choose a reason for hiding this comment

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

see comments above

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure! I did it here: 0a4d54a

@@ -78,7 +91,7 @@ mod output {
impl TableOutputHandler for ListConnectors {
/// table header implementation
fn header(&self) -> Row {
Row::from(["CONNECTOR", "Visibility"])
Row::from(["Connector", "Visibility", "Released"])
Copy link
Contributor

Choose a reason for hiding this comment

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

Published

Copy link
Contributor

Choose a reason for hiding this comment

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

Probably Connector -> Versions

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated the Published already! About Versions I think that would belong to a new column doesn't it? Perhaps as an iteration from this we could split name from version and use a column for versions only?

@EstebanBorai EstebanBorai requested a review from sehz October 4, 2024 00:48
.expect("failed to sub days");
let formatted = make_fixed_time_elapsed_fmtr().time_since_str(date, false);

assert_eq!(formatted, "1989/12/10");
Copy link
Contributor

@sehz sehz Oct 4, 2024

Choose a reason for hiding this comment

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

why it is different from determines_exact_date
Shouldn't it be 1989/12/10 00:00::00 UTC ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Its just simpler to keep it compact, an output including time and thus timezone (e.g. 1989/12/10 00:00::00 UTC) would only appear if user uses exact-date.

Do you rather using exact date as fallback for longer periods?

Copy link
Contributor

Choose a reason for hiding this comment

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

I thought it automatically converts to exact if exceed certain time. This is only 7 days, so shouldn't be a week or something like that?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We could have week counter yeah, perhaps we should set boundaries?
I think we could render exact date for more than a year? Otherwise show months, weeks and days?

Copy link
Contributor

Choose a reason for hiding this comment

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

There is well established library like: https://www.npmjs.com/package/humanize-duration that specifies behavior

Self { bot }
}

pub fn time_since_str(&self, date: DateTime<Utc>, exact: bool) -> String {
Copy link
Contributor

Choose a reason for hiding this comment

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

should add docs on rule

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Here I have added docs: e96128c, I will use such statements in the docs as guide for free-form expressions.

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.

CDK: List release date
3 participants