Fixed TimeStamp resolution #4
Conversation
| @@ -5,8 +5,8 @@ import "testing" | |||
| func TestTimeStamp(t *testing.T) { | |||
| t.Run("default", func(t *testing.T) { | |||
| result := TimeStamp() | |||
| if result <= 0 { | |||
| t.Errorf("Expected non-zero timestamp, got %v", result) | |||
| if result < 10000000000 { | |||
There was a problem hiding this comment.
Just to understand why you changed this: couldn't this still be <= 0 as all that it checks if the value is non-zero and the returned resolution does not really matter?
There was a problem hiding this comment.
Changed to make a basic check if resolution is in milliseconds. If >=1e10 is returned, than it's definitely milliseconds (1970 year), because if it's in seconds - it's 2286 year. I don't think anybody will use this library in 2286.
Yep, it's kind of a magic number. But there is no better way to check for resolution. Also, you can remove it, if you want, because I don't think anybody will break this simple function :).
There was a problem hiding this comment.
Yeah, I think that makes sense. Let's keep it like this.
|
Thanks for the PR, that does indeed look like a bug. It's strange that Chatbase never complained, but well. Travis should be failing as it is lacking the secrets needed to build on your branch, but I will checkout your branch locally and run the tests here just to be sure. |
|
Yeah, integration tests pass just fine when the correct environment variable is set. |
|
I merged and tagged this as |
|
Whoops, I just noticed that returning |
|
Actually I think the right thing to do would be to do the following:
|
|
I rolled back the type changes and only left the timestamp resolution fix in |
|
Yes, it's a breaking change and it doesn't matter I think this library is not very popular and nobody used |
It will start behaving as expected (which will be unexpected, but well...), but it won't stop compiling when using
Well I for one use it and it broke my build :) so that alone is definitely worth sticking to semver. |
|
So Thanks again! |
According to the API, timestamps should be in milliseconds:
int, <required> milliseconds since the UNIX epoch, used to sequence messages. (must be within previous 30 days). Currently they are in seconds, which breaks messages sequence at least in Transcripts.