-
Notifications
You must be signed in to change notification settings - Fork 77
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
fix(stdlib): human time parse duration error for decimals #1223
base: main
Are you sure you want to change the base?
Conversation
334f9b6
to
03f803c
Compare
Thank you for trying for fixing this! Is it possible to support multi-units duration in the implementation as well? |
Good idea, it could have been in a separate function but I am fine to include in this one as long as we include some more tests 👍 |
Test cases for multi units duration are already present ig. Any sample test case? Just wanted to understand what kind of new test cases needs to be added just so that we are on the same page |
Perhaps testcase for multi-unit and fractional time duration? |
Ahh got it |
Added. Thanks @titaneric for the logic |
@titaneric if we are adding support to the existing implementation to handle multiple units, is there any reason to keep the implementation using |
I am not sure, to be honest. I think we could remove the humantime crate this time. |
want: Err("unknown unit format: 'w'"), | ||
want: Ok(0.000_001_653_439_153_439_153_5), |
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.
Not sure if we want to support weeks. Either we remove support for both w_ns and s_w or keep it as it is. Let me your thoughts
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.
I think week is a common unit, and I think we could support it
I agree, it seems like we need to handle the parsing ourselves. |
Credits to @titaneric for original implementation
Summary
New humantime parse duration function doesn't support decimals. So falling back to old method(regex) when ht_parse_duration fails
Change Type
Is this a breaking change?
How did you test this PR?
Does this PR include user facing changes?
our guidelines.
Checklist
run
dd-rust-license-tool write
and commit the changes. More details here.References