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

TimeSpan humanization precision skips units after the largest one when they're zero until it finds a non-zero unit #339

Closed
RedwoodForest opened this issue Oct 20, 2014 · 5 comments · Fixed by #340
Labels

Comments

@RedwoodForest
Copy link

I think this is best demonstrated by example. Take this expression: TimeSpan.FromHours(1.0001).Humanize(precision:2)

I was expecting the output to be 1 hour (omitting minutes) because I understood precision to mean the granularity of time I wanted to display to the user (in this case the largest two time units, not displaying any units with zero value after the largest)), but the actual output is 1 hour, 360 milliseconds, displaying the largest two non-zero time units skipping over all zero-value units in between.

My use case is wanting to tell my users roughly how much time the TimeSpan represents with a little more precision than the default of only the largest unit. For example if the largest unit was an hour they might be interested in the number of minutes, but I don't think they need to know the number of milliseconds if there aren't any minutes or seconds.

Would it be possible to support my use case in this library somehow (either via modification of the existing behavior or adding a new feature)?

Also this behavior (skipping over zero-value time units) doesn't seem to be documented in any of the examples. If the current behavior isn't changed could the documentation be updated to prevent other people from being surprised the same way I was?

@RedwoodForest RedwoodForest changed the title TimeSpan humanization precision skips units after the largest one when they're 0 until it finds a non-0 unit TimeSpan humanization precision skips units after the largest one when they're zero until it finds a non-zero unit Oct 20, 2014
@MehdiK MehdiK added the bug label Oct 21, 2014
@MehdiK
Copy link
Member

MehdiK commented Oct 21, 2014

Thanks a lot for reporting this. You are right: precision does mean the time granularity and in this case it should only show the hour. This is a bug.

@mexx
Copy link
Collaborator

mexx commented Oct 21, 2014

I've provided a fix, unfortunately it is a breaking change in the functionality. It should be properly announced.

@RedwoodForest
Copy link
Author

Thanks to both of you. That it's a breaking change and will need to be announced makes complete sense.

@MehdiK
Copy link
Member

MehdiK commented Oct 22, 2014

Thanks for the fix @mexx.

That's very interesting. I actually didn't look at the code when I said it's a bug - it was just my common sense reaction to it. I just had a look at your fix and I remembered that it was actually how I implemented it and in the hindsight I don't like the current behavior!

I don't want to bump the major for this small fix and don't want to delay this until v2; but I also don't want to break people's code. In fact, this might be the expected behavior for some. So I think we should keep this backward compatible. So I would like to suggest to either change the behavior using a config value (my preference) or add an overload to enforce the new behavior (both keeping the current behavior intact). Thoughts?

@mexx
Copy link
Collaborator

mexx commented Nov 4, 2014

Sorry for being off for a while.
I've implemented the later option, as optional parameter considerEmptyParts.
Any comments are welcome.

mexx added a commit to mexx/Humanizer that referenced this issue Nov 4, 2014
Borzoo pushed a commit to Borzoo/Humanizer that referenced this issue Dec 23, 2014
Borzoo pushed a commit to Borzoo/Humanizer that referenced this issue Dec 23, 2014
Borzoo pushed a commit to Borzoo/Humanizer that referenced this issue Dec 23, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants