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 off by one error in RelTime #65

Merged
merged 1 commit into from Nov 11, 2017

Conversation

Projects
None yet
2 participants
@dustin
Owner

dustin commented Nov 10, 2017

This change fixes a user reported issue where the RelTime formatter picks the wrong magnitude at the exact nanosecond it changes.

User report:

package main

import (
	"fmt"
	"time"

	"google3/base/go/google"
	"google3/third_party/golang/humanize/humanize"
)

func main() {
	google.Init()
	fmt.Println(humanize.RelTime(time.Unix(0, 0), time.Unix(7*24*60*60, 0), "ago", ""))
	fmt.Println(humanize.RelTime(time.Unix(0, 0), time.Unix(7*24*60*60, 1), "ago", ""))
	fmt.Println(humanize.RelTime(time.Unix(0, 0), time.Unix(14*24*60*60, 0), "ago", ""))
	fmt.Println(humanize.RelTime(time.Unix(0, 0), time.Unix(14*24*60*60, 1), "ago", ""))
}

produces the following output:

7 days ago
1 week ago
1 week ago
2 weeks ago

@dustin dustin requested a review from dmitshur Nov 10, 2017

@dustin

This comment has been minimized.

Show comment
Hide comment
@dustin

dustin Nov 10, 2017

Owner

Travis reports t.Helper fails on a bunch of the builders. I've removed that change from this PR.

Owner

dustin commented Nov 10, 2017

Travis reports t.Helper fails on a bunch of the builders. I've removed that change from this PR.

Show outdated Hide outdated times_test.go Outdated
@dustin

This comment has been minimized.

Show comment
Hide comment
@dustin

dustin Nov 11, 2017

Owner

Added some tests and renamed these conditions. Does this behavior seem reasonable?

Owner

dustin commented Nov 11, 2017

Added some tests and renamed these conditions. Does this behavior seem reasonable?

@dustin

This comment has been minimized.

Show comment
Hide comment
@dustin

dustin Nov 11, 2017

Owner

(accidentally pushed to head -- fixed it)

Owner

dustin commented Nov 11, 2017

(accidentally pushed to head -- fixed it)

@dmitshur

dmitshur approved these changes Nov 11, 2017 edited

LGTM. See inline comment for minor nitpicky suggestion, and below for some background on this change.

The sort.Search code used to do return magnitudes[i].d > diff, before it was changed to use >= in #26.

I brought up a concern in that PR and made an argument that > seemed to be more correct/better fitting than >=. See #26 (comment) specifically, as well as second paragraph of #26 (comment). It looks like my comment got lost there. This PR seems to apply that change now, hence my LGTM.

That said, I'm a little surprised changing magnitudes[i].D >= diff to magnitudes[i].D > diff doesn't break any of the existing tests, because it did back when #26 was being reviewed (according to the last paragraph of my comment there):

I can see simply changing this >= to > breaks the other tests. Based on the above, I think the original > here is correct, and tests should be updated to match.

I wonder what happened to those tests...

Show outdated Hide outdated times_test.go Outdated
Fix for reported off-by-one (nanosecond) error in RelTime
User reported this error with a test case.  I'm not entirely sure how
it's different from existing cases, but it does appear to do something
undesirable.
@dmitshur

This comment has been minimized.

Show comment
Hide comment
@dmitshur

dmitshur Nov 11, 2017

Collaborator

Heh, I've been using the go font for go code, so I don't remember what alignment looks like. :)

Me too! :D Wait, do you mean you use the actual "Go" font, the non-monospaced one? I use "Go Mono".

Apologies for missing that comment. github's code review is a little foreign to me. I'm used to the ones where you have to actually take action when someone calls something out (that is, if I ignore it, I have to at least do it intentionally). It does seem easy to drop stuff here.

Makes sense; no worries.

Not sure about the tests, though. I'd've hoped a lot would break changing a >= to a >

I'm guessing it's related to that PR #26 being merged as df564ff with some additional changes from your similar PR #33 merged as 3143592 on top. Oh well, I'm not too worried about it by now.

Collaborator

dmitshur commented Nov 11, 2017

Heh, I've been using the go font for go code, so I don't remember what alignment looks like. :)

Me too! :D Wait, do you mean you use the actual "Go" font, the non-monospaced one? I use "Go Mono".

Apologies for missing that comment. github's code review is a little foreign to me. I'm used to the ones where you have to actually take action when someone calls something out (that is, if I ignore it, I have to at least do it intentionally). It does seem easy to drop stuff here.

Makes sense; no worries.

Not sure about the tests, though. I'd've hoped a lot would break changing a >= to a >

I'm guessing it's related to that PR #26 being merged as df564ff with some additional changes from your similar PR #33 merged as 3143592 on top. Oh well, I'm not too worried about it by now.

@dustin

This comment has been minimized.

Show comment
Hide comment
@dustin

dustin Nov 11, 2017

Owner

Yeah, I started using the non-mono one. We don't have hard line length limitations at work, and go doesn't do very fancy alignment, so I gave it a shot to see how I'd like it. It's not that bad, but it was a pretty big leap to program without a fixed width font. I still used a fixed with font for everything else.

Owner

dustin commented Nov 11, 2017

Yeah, I started using the non-mono one. We don't have hard line length limitations at work, and go doesn't do very fancy alignment, so I gave it a shot to see how I'd like it. It's not that bad, but it was a pretty big leap to program without a fixed width font. I still used a fixed with font for everything else.

@dustin dustin merged commit bb3d318 into master Nov 11, 2017

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
@dmitshur

This comment has been minimized.

Show comment
Hide comment
@dmitshur

dmitshur Nov 11, 2017

Collaborator

Not sure about the tests, though. I'd've hoped a lot would break changing a >= to a >

I'm guessing it's related to that PR #26 being merged as df564ff with some additional changes from your similar PR #33 merged as 3143592 on top.

Notably, 3143592 removed TestRelTimeMagnitudes, which is where the tests that failed on magnitudes[i].d > diff were located, I suspect.

Collaborator

dmitshur commented Nov 11, 2017

Not sure about the tests, though. I'd've hoped a lot would break changing a >= to a >

I'm guessing it's related to that PR #26 being merged as df564ff with some additional changes from your similar PR #33 merged as 3143592 on top.

Notably, 3143592 removed TestRelTimeMagnitudes, which is where the tests that failed on magnitudes[i].d > diff were located, I suspect.

@dustin

This comment has been minimized.

Show comment
Hide comment
@dustin

dustin Nov 11, 2017

Owner

Interesting. I do wish go had a better quickcheck. I've found lots of these annoying edge case bugs with property tests. I've got a few good ones at work, but it's so much harder to do it well in go than it is in haskell that you can get lost in the details.

Owner

dustin commented Nov 11, 2017

Interesting. I do wish go had a better quickcheck. I've found lots of these annoying edge case bugs with property tests. I've got a few good ones at work, but it's so much harder to do it well in go than it is in haskell that you can get lost in the details.

@dmitshur

This comment has been minimized.

Show comment
Hide comment
@dmitshur

dmitshur Nov 11, 2017

Collaborator

Yeah, I started using the non-mono one. We don't have hard line length limitations at work, and go doesn't do very fancy alignment, so I gave it a shot to see how I'd like it. It's not that bad, but it was a pretty big leap to program without a fixed width font. I still used a fixed with font for everything else.

I see, cool. :) As far as I know, Rob Pike uses a non-mono font for coding too. I might give it a try for a longer period of time someday.

Either way, I do enjoy the Go fonts quite a bit (see my mini-review of it here, lol). In addition to my code editor, I also use it on my personal site, and I managed to get it into maintserve as well, hehe.

Collaborator

dmitshur commented Nov 11, 2017

Yeah, I started using the non-mono one. We don't have hard line length limitations at work, and go doesn't do very fancy alignment, so I gave it a shot to see how I'd like it. It's not that bad, but it was a pretty big leap to program without a fixed width font. I still used a fixed with font for everything else.

I see, cool. :) As far as I know, Rob Pike uses a non-mono font for coding too. I might give it a try for a longer period of time someday.

Either way, I do enjoy the Go fonts quite a bit (see my mini-review of it here, lol). In addition to my code editor, I also use it on my personal site, and I managed to get it into maintserve as well, hehe.

@dmitshur dmitshur deleted the rello branch Nov 11, 2017

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