- Notifications
You must be signed in to change notification settings - Fork1k
chore: add clock pkg for testing time#13461
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
spikecurtis commentedJun 4, 2024 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
This stack of pull requests is managed by Graphite.Learn more about stacking. Join@spikecurtis and the rest of your teammates on |
e6c30e4
to8caaed8
CompareThere 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.
Nice work! I'm happy to see how the modified test cases in pubsub nicely demonstrates the utility of the trap concept. I didn't re-read the RFC but this implementation largely matches my recollection of the approach.
Considering this is likely going to be its own library in the future, I'd love to see somefunc Example...
s that demonstrate the functionality and use-cases. Some in-package tests would be nice too.
Uh oh!
There was an error while loading.Please reload this page.
clock/clock.go Outdated
// NewContextTicker is a convenience function that calls f on the interval d until either the | ||
// given context expires or f returns an error. Callers may call Wait() on the returned Waiter | ||
// to wait until this happens and obtain the errror. | ||
NewContextTicker(ctx context.Context,d time.Duration,ffunc()error,tags...string)Waiter |
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.
How would you feel about using functional options instead of...string
? An alternative to having this convenience function would be to accept the following signature:
NewTicker(d time.Duration, f func() error, opts ...Option)
and you'd pass itclock.WithContext(ctx)
or `clock.Tags("tag1", "tag2") as options.
I think this could result in a slightly cleaner and more flexible API, as well as the added verbosity/explicitness can help with code reads (i.e. "what are these strings?").
In this scenario, I imagine the return signature ofNewTicker
could include all three methodsStop
,Reset
andWait
.
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.
Firstly, maybe "ContextTicker" isn't the best name. It's not a ticker that optionally takes a context, it's a ticker calls the functionf
on the ticks, rather than just sending the ticks to a channel. Sort of likeAfterFunc
is a special timer that calls a function. "TickerFunc"?
Secondly, the idea is to include tags onevery call in theclock
interface, and all the timer/ticker calls as well. With the exception of this ContextTicker/TickerFunc, the idea is to verbatim replicate the functionality of thetime
package, not enhance it. So, I literally cannot think of another option we'd want other than tags, and even if I could, it's unlikely to apply toevery call, so adding it to a genericOption
interface would muddy the waters. I think variability is actually a downside in this case, and we should keep it simple.
Lastly, I'm not a fan of the verbosity ofclock.Tags("tag1")
. The signature saystags ...string
, so I feel like that's clear enough once you jump to the definition.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
I really like how this is looking so far! 👍 Just a few suggestions on my end. |
5da53ab
toe243711
Comparee243711
to63c9374
Compare
Uh oh!
There was an error while loading.Please reload this page.
Adds a package for testing time/timer/ticker functions. Implementation is limited to
NewTimer
andNewContextTicker
, but will eventually be expanded to alltime
functions from the standard library as well ascontext.WithTimeout()
,context.WithDeadline()
.Replaces
benbjohnson/clock
for the pubsub watchdog, as a proof of concept.Eventually, as we expand functionality, we will replace most time-related functions with this library for testing.