-
Notifications
You must be signed in to change notification settings - Fork 2.5k
DOC-5472 time series doc examples #3443
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
base: master
Are you sure you want to change the base?
DOC-5472 time series doc examples #3443
Conversation
Hello @andy-stark-redis , I will review this till end of week. Thank you. |
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.
@andy-stark-redis I left some styling suggestions on the first occurrences of the patterns that I observed. Feel free to address them for the rest of the examples. Mainly - remove empty lines before checking the error and if the method arguments are on multiple lines, keep them one per line. Other than that, looks good to me.
doctests/timeseries_tut_test.go
Outdated
res6, err := rdb.TSAddWithArgs( | ||
ctx, | ||
"thermometer:3", | ||
1, 10.4, |
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.
1, 10.4, | |
1, | |
10.4, |
Maybe it is slightly more readable on separate lines, let me know what you think.
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.
@ndyakov Yeah, I'm not 100% sure about this. The reason I formatted them like this was that the timestamp and value together form a logical pair. It's a bit like the following code from the hash examples:
hashFields := []string{
"model", "Deimos", // Key and value are just strings in the array but
"brand", "Ergonom", // this formatting emphasises the key-value pairs.
"type", "Enduro bikes",
"price", "4972",
}
But if it doesn't look like normal Go style to do it this way for the timestamps then I'll put them on separate lines.
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 see your point. Since this is supposed to be part of the documentation, I assume it would be better to look similar to other language that have similar APIs. I personally would prefer separate lines for the current code, but do see the benefit of having them combined in the hash example that you shared.
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.
OK, I'll use separate lines for this example. Thanks for clarifying this :-)
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.
Fixed.
DOC-5472
Go versions of the Python examples in the time series doc page.
Couple of things to note:
math.MaxInt64
instead of-
and+
for minimum/maximum timestamps is OK (eg, line 211), but if there's a better way to specify this then let me know.TSMGet
/TSMRange
examples return maps, so the ordering keys in nondeterministic. I've used code that's a bit more complicated than usual to print out the results in a deterministic way (eg, the example at line 438). If there's a simpler or more idiomatic way to handle this situation then I'll use that instead.