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

Add years and months to the human date formatter #425

Merged
merged 12 commits into from
Oct 9, 2024

Conversation

ValentinLeTallec
Copy link
Contributor

Hi, the current implementation of the human formatter uses days as its biggest unit, I added years and months

Before :

>>> 2 yr + 50 days -> human
  human(2 year + 50 day)
    = 780 days + 11 hours + 37 minutes + 30.103680 seconds    [String]

>>> 2 yr + 50 s -> human
  human(2 year + 50 second)
    = 730 days + 11 hours + 38 minutes + 20.103680007 seconds    [String]

After :

>>> 2 yr + 50 days -> human
  human(2 year + 50 day)
    = 2 years + 1 month + 19 days + 13 hours + 30 minutes + 56.246 seconds    [String]

>>> 2 yr + 50 s -> human
  human(2 year + 50 second)
    = 2 years + 50 seconds    [String]

P.S Love numbat ^^

@ValentinLeTallec
Copy link
Contributor Author

I'm not quite sure what the error in the CICD is about 🤔
I think I will need some insight from you before I am able to fix it.

thread 'modules_are_self_consistent' panicked at 'assertion failed: result.is_ok()', numbat/tests/prelude_and_examples.rs:24:5
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

failures:
    modules_are_self_consistent

test result: FAILED. 5 passed; 1 failed; 0 ignored; 0 measured; 0 filtered out; finished in 1.00s

error: test failed, to rerun pass `-p numbat --test prelude_and_examples`
Error: Process completed with exit code 101.

@triallax
Copy link
Contributor

triallax commented May 13, 2024

Thanks for your contribution!

Days, minutes, hours, and seconds have a more or less constant value, unlike months and years which can have different lengths (notwithstanding that we have them in Numbat as average values), so I think we might want to think twice about adding them to human due to the lack of precision. But to be honest, given that we already have year and month as average values, the cat may be out of the bag for this already.

@sharkdp
Copy link
Owner

sharkdp commented May 13, 2024

Also, please note that we have an open PR targeting human here: #400

@sharkdp
Copy link
Owner

sharkdp commented May 13, 2024

I'm not quite sure what the error in the CICD is about 🤔

This test makes sure that all modules can be used in a standalone way. You can reproduce the error in the following way: start numbat with the --no-prelude option. Then, try to import the module (in this case: use datetime::human). You will probably get an error like "sidereal_day not defined". This can be fixed by including the necessary modules in numbat/modules/datetime/human.nbt.

@ValentinLeTallec
Copy link
Contributor Author

Days, minutes, hours, and seconds have a more or less constant value, unlike months and years which can have different lengths (notwithstanding that we have them in Numbat as average values), so I think we might want to think twice about adding them to human due to the lack of precision. But to be honest, given that we already have year and month as average values, the cat may be out of the bag for this already.

Yeah, I do think it makes sense considering those units already exist in numbat (and the documentation does have proper warnings about them being average value).

To me -> human feels more about having an easy-to-read duration than a precise one. When more precision is needed, I would naturally gravitate toward conversions to a specific unit.

As a side note on the subject, I was wondering if, for durations greater than a month, it would be more ergonomic to hide the hours, minutes and seconds, as 1 year + 11 months + 6 days + 22 hours + 51 minutes + 33.65 seconds ago get somewhat long (maybe 1 year + 11 months + 6.95 days ? or just 1 year + 11 months + 7 days by rounding)

Also, please note that we have an open PR targeting human here: #400

I added the management of the negative time to this pull request (I have used ago because that what is used in #400)

This test makes sure that all modules can be used in a standalone way. You can reproduce the error in the following way: start numbat with the --no-prelude option. Then, try to import the module (in this case: use datetime::human). You will probably get an error like "sidereal_day not defined". This can be fixed by including the necessary modules in numbat/modules/datetime/human.nbt.

Yep, thanks, I solved it ^^

@sharkdp sharkdp mentioned this pull request May 15, 2024
@sharkdp
Copy link
Owner

sharkdp commented May 15, 2024

I'm not completely sure about this. As @triallax mentioned, human is a precise transformation for now. Once we add months and years, the interpretation is not completely clear.

I'm with you (@ValentinLeTallec) that we're often just interested in a rough estimation. But in this case, I can already call 10000 days -> months or 1e9 days -> years. I don't need human for that. I do need it if I am interested in splitting a decimal timespan into hours/minutes/seconds (7000 seconds -> human). Here, I actually do care how many minutes and seconds I get, not just how many hours. Otherwise, I could have called (7000 seconds -> hours).

We may want to have that same feature (split a time span into mixed units) for years/months/days as well, but then the imprecision bothers me.

@ValentinLeTallec

This comment was marked as duplicate.

@ValentinLeTallec
Copy link
Contributor Author

Yeah, it does make sens to keep precise measurements for the use case you are describing.

I think the current version is more geared toward a human_time and what I had in mind is more of an human_date.

My use case would be more something like :

>>> date("2025-12-12") - today() -> human
  human(date("2025-12-12") - today())
    = 1 year + 6 months + 27 days    [String]

Maybe we could separate it in two functionalities ?

@sharkdp
Copy link
Owner

sharkdp commented May 27, 2024

My use case would be more something like :

>>> date("2025-12-12") - today() -> human
  human(date("2025-12-12") - today())
    = 1 year + 6 months + 27 days    [String]

But in this exact use case, the imprecision actually bothers me. I don't want this to be off by 1 day.

@ValentinLeTallec
Copy link
Contributor Author

ValentinLeTallec commented May 27, 2024

Then maybe more like that :

>>> date("2025-12-12") - today() -> human_date
  human(date("2025-12-12") - today())
    = 1 year + 6 months + 16.18 days    [String]

?

@sharkdp
Copy link
Owner

sharkdp commented Jun 23, 2024

Maybe we could keep the current format, but add something like "approx. 1 year, 6 months, 16 days" in parenthesis after the default output in case the time is larger than, say, 2 months?

@ValentinLeTallec
Copy link
Contributor Author

Yep, that does sounds good to me 🙂

@ValentinLeTallec
Copy link
Contributor Author

ValentinLeTallec commented Jul 13, 2024

For durations larger than 2 months, do you prefer :

assert((2 month          -> human) == "60 days + 20 hours + 58 minutes + 7.509 seconds (approx. 2 months)")
assert((3 yr + 2 month   -> human) == "1156 days + 14 hours + 24 minutes + 22.664 seconds (approx. 3 years + 2 months)")
assert((10 yr + 2 s      -> human) == "3652 days + 10 hours + 7 minutes + 32.518 seconds (approx. 10 years + 2 seconds)")

or

assert((2 month          -> human) == "60.8737 days (approx. 2 months)")
assert((3 yr + 2 month   -> human) == "1156.6 days (approx. 3 years + 2 months)")
assert((10 yr + 2 s      -> human) == "3652.42 days (approx. 10 years + 2 seconds)")

?

I personally prefer the second option as it is more concise and it avoids computing 2 times the days/hours/minutes/seconds which is more or less needed for the first option (at least in my implementation)

@sharkdp
Copy link
Owner

sharkdp commented Jul 16, 2024

I also prefer the second option, but I would only show months and years in parenthesis. For times smaller than 1 year (but larger than 2 months), I would only show months. For times smaller than 100 years, I would show years and months. For times larger than 100 years, I would only show years. Something like 10 years + 2 s looks correct for the example you gave, but that is a completely misleading precision for users who don't know how exactly the year is defined in Numbat.

@ValentinLeTallec
Copy link
Contributor Author

So to sum-up, we would have :

  • time < 2 months
    • only show months
    • 10 days + 2 hours + ...
  • 2 months < time < 1 year
    • only months
    • 65.12 days (approx. 2 months)
  • 1 year < time < 100 years
    • only years and months
    • 710 days (approx. 1 year + 11 months)
  • 100 year < time
    • only years
    • 71000 days (approx. 194 years)

I feel like it would be in line with the desired accuracy to also display days for 2 months < time < 1 year (65.12 days (approx. 2 months + 4 days))

@sharkdp
Copy link
Owner

sharkdp commented Jul 19, 2024

time < 2 months

* only show months

* `10 days + 2 hours + ...`

Why "only show months" here? I would think that we only show the latter part in this case.

I feel like it would be in line with the desired accuracy to also display days for 2 months < time < 1 year (65.12 days (approx. 2 months + 4 days))

Again, I'm skeptical. Imagine someone runs something like this:

>>> date("2023-04-15") - date("2023-02-15") -> human

    = "58 days + 23 hours"    [String]

If we do like you propose, we would not add "approx. 2 months", but "approx. 1 month + 29 days" instead. That is more correct if you know exactly what "1 month" represents in Numbat. But it's confusing if you interpret it in terms of calendar months.

@ValentinLeTallec
Copy link
Contributor Author

Why "only show months" here? I would think that we only show the latter part in this case.

My bad, it's a typo from copy-pasting, I edited my message to strike through it.

If we do like you propose, we would not add "approx. 2 months", but "approx. 1 month + 29 days" instead. That is more correct if you know exactly what "1 month" represents in Numbat. But it's confusing if you interpret it in terms of calendar months.

But then what about cases where you are not so near one of the edges of the month ?

Having :

>>> date("2023-04-15") - date("2023-01-01") -> human
    = "103 days + 23 hours (approx. 3 months)"    [String]

rather than :

>>> date("2023-04-15") - date("2023-01-01") -> human
    = "103 days + 23 hours (approx. 3 months + 12 days)"    [String]

feels like a pretty big loss of accuracy in the approximation.

But it's confusing if you interpret it in terms of calendar months.

I think someone expecting calendar months will already be disturbed by the + 23 hours and that adding the month/day approximation only hint toward the right direction.

Especially, thanks to your previous suggestion, as we now have clear indication (approx.) that the number in parentheses is an approximation.

While

>>> date("2023-04-15") - date("2023-01-15") -> human
    = 2 months + 29 days

was indeed confusing, the new version is much clearer on what is an approximation

>>> date("2023-04-15") - date("2023-01-15") -> human
    = "89 days + 23 hours (approx. 2 months + 29 days)"

@sharkdp
Copy link
Owner

sharkdp commented Jul 20, 2024

Sorry for dragging this out for so long.

But then what about cases where you are not so near one of the edges of the month ?

Final alternative suggestion: how about decimal months? Rounded to one decimal, so we would see things like 1.4 months or 2.9 months)

@ValentinLeTallec
Copy link
Contributor Author

Yep, let do that

time < 2 months
    show all
    10 days + 2 hours + ...
2 months < time < 1 year
    only months (1 decimal)
    65.12 days (approx. 2.2 months)
1 year < time < 100 years
    only years and months
    710 days (approx.  1 year + 11 months)
100 year < time
    only years
    71000 days (approx. 194 years)
@ValentinLeTallec
Copy link
Contributor Author

New version with what we discussed !

@@ -20,19 +20,27 @@ assert_eq((1 day -> human), "1 day")
assert_eq((1.37 day -> human), "1 day + 8 hours + 52 minutes + 48 seconds")

assert_eq((1 week -> human), "7 days")
assert_eq((1.5 weeks -> human), "10 days + 12 hours")
Copy link
Owner

Choose a reason for hiding this comment

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

Why is this test missing now?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Because of #589 😅 , but with your answer there, I think I could improve the code here to have it work too 🙂 .

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, fixed in 89a141b

Copy link
Owner

@sharkdp sharkdp left a comment

Choose a reason for hiding this comment

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

Thank you for the updates! Great change 👍

@sharkdp sharkdp merged commit a4eadb6 into sharkdp:master Oct 9, 2024
15 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.

3 participants