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

Clarify duration values as integers #10356

Closed
wants to merge 1 commit into from

Conversation

jhlodin
Copy link
Contributor

@jhlodin jhlodin commented Dec 20, 2021

Clarify that duration type values must be integers.

Copy link
Member

@mosabua mosabua left a comment

Choose a reason for hiding this comment

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

Currently it seems decimal values actually work. I suggest that we update the docs like this (only suggesting usage of integers), and then as a follow up also change the code so that only integer values are supported.

This is necessary to avoid confusion such as what is 0.5 months? Or even others like 1.30m ... is that one minute and 30 seconds .. so really 1.5m ? E.g. in this case users should just use 90s.

Wdyt @electrum ? Also .. this might have to be done in Airlift..

@jhlodin jhlodin requested a review from hashhar January 4, 2022 19:42
Copy link
Member

@hashhar hashhar left a comment

Choose a reason for hiding this comment

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

The existing docs are correct.

Changing allowed values is not feasible due to multiple reasons IMO:

  1. It's a breaking change without a good-enough justification.
  2. It might not be straightforward to limit to only integer values since a Duration object can constructed in many ways - only one of which is via human readable string.

I'll defer to @electrum for a decision on this.

@martint
Copy link
Member

martint commented Jan 4, 2022

This is necessary to avoid confusion such as what is 0.5 months

Duration only supports units up to days, for that specific reason. So there's no confusion there.

1.30m

Not sure what's ambiguous about that. It's 1.3 minutes (just a fractional number), not 1:30 minutes.

one minute and 30 seconds .. so really 1.5m ? E.g. in this case users should just use 90s.

What's wrong with using 1.5 minutes? The natural magnitude of a setting (what you intuitively think about) might be in terms of minutes, or hours, or days. For instance, what if you want to represent "a tenth of a day"? If you can't use fractional numbers, you can't say "0.1d", or even the equivalent in hours (i.e., "2.4h"). You'd need to say "144m", which is hard to mentally relate to the length of a day.

Also .. this might have to be done in Airlift..

No, that was an intentional design choice in Airlift, and it needs to be that way for may other reasons.

@jhlodin
Copy link
Contributor Author

jhlodin commented Jan 6, 2022

Closing this PR, will follow up shortly with another PR that clarifies how decimal duration values are handled.

@jhlodin jhlodin closed this Jan 6, 2022
@jhlodin jhlodin deleted the jl/duration-prop-integer branch January 6, 2022 15:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Development

Successfully merging this pull request may close these issues.

None yet

4 participants