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

[bugfix] duration: parser: Support ms durations in .NET syntax #3276

Closed
wants to merge 2 commits into
base: develop
from

Conversation

Projects
None yet
5 participants
@not-an-aardvark
Contributor

not-an-aardvark commented Jul 4, 2016

Fixes #3266

Note: The rounding behavior described in #3266 (comment) is not currently included in this PR, because it would break a number of existing tests. For example, with the proposed rounding behavior, the duration parsed in this test would have seconds = 5 and milliseconds = 0.

I can add the rounding behavior and change those tests if desired, but that might be considered a breaking change, so I thought it was worth bringing up the issue first. At the moment, extra millisecond digits are simply truncated rather than being rounded.

edit: The rounding behavior has been added to this PR.

@maggiepint

This comment has been minimized.

Member

maggiepint commented Jul 4, 2016

I would make it round. I understand that it's kind of a breaking change in the sense that the behavior changes, but it really does make more sense. If we have the data for the later decimal places we should use it. Maybe some people have to change their unit tests in some project as a result, but I'd consider that acceptable to fix what really is broken.

@not-an-aardvark

This comment has been minimized.

Contributor

not-an-aardvark commented Jul 4, 2016

Ok, I added the rounding behavior.

@FallingSnow

This comment has been minimized.

FallingSnow commented Jul 10, 2016

Any update on this?

@maggiepint

This comment has been minimized.

Member

maggiepint commented Jul 31, 2016

LGTM. @mj1856 this is your specialty - you like this?

@mj1856

This comment has been minimized.

Member

mj1856 commented Aug 1, 2016

Yep. Looks great. Thanks!

@mj1856 mj1856 modified the milestone: 2.15.0 Aug 5, 2016

@ichernev

This comment has been minimized.

Contributor

ichernev commented Sep 1, 2016

Merged in 6848096

@ichernev ichernev changed the title from Ensure that millisecond durations are parsed from .NET timestamps to [bugfix] duration: parser: Support ms durations in .NET syntax Sep 1, 2016

@ichernev ichernev closed this Sep 1, 2016

ichernev added a commit that referenced this pull request Sep 1, 2016

Merge pull request #3276 from not-an-aardvark:issue3266
[bugfix] duration: parser: Support ms durations in .NET syntax

@not-an-aardvark not-an-aardvark deleted the not-an-aardvark:issue3266 branch Sep 14, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment