-
Notifications
You must be signed in to change notification settings - Fork 954
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 time span humanization precision skipping #340
Conversation
What do you think about the suggestions at #339 (comment)? |
/// <returns></returns> | ||
public static string Humanize(this TimeSpan timeSpan, int precision = 1, CultureInfo culture = null) | ||
public static string Humanize(this TimeSpan timeSpan, int precision = 1, CultureInfo culture = null, bool considerEmptyParts = false) |
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 am not so sure about the optional param. It's not clear why one would want to use that and it could actually be used without passing the precision
, timespan.Humanize(considerEmptyParts: true)
, which is not good.
I'd suggest adding the following three methods:
public static string Humanize(this TimeSpan timeSpan, CultureInfo culture = null)
public static string Humanize(this TimeSpan timeSpan, int precision, bool ignoreEmptyUnits = false, CultureInfo culture = null)
[Deprecated]
public static string Humanize(this TimeSpan timeSpan, int precision, CultureInfo culture = null)
A couple of things to highlight:
- precision on the overload is not optional anymore
considerEmptyParts
has been renamed toignoreEmptyUnits
but the default value is left as false. This means that the logic should change to by default not ignore empty units which will keep the change backward compatible for those who hadn't passed the culture in. Due to the name change the implementation needs to be reversed too.
Thanks for the change @mexx. I left a comment on the method signature. What do you think? Also please add the addition to the readme file. |
I've changed the implementation and made the parameters in the overload mandatory, TimeSpan.FromMilliseconds(3603001).Humanize(3, countEmptyUnits:true) With TimeSpan.FromMilliseconds(3603001).Humanize(3, ignoreEmptyUnits:false) as by default of current behavior empty units would be ignored. @MehdiK What do you think? |
I like it. Thanks. |
fix time span humanization precision skipping
fixes #339