-
Notifications
You must be signed in to change notification settings - Fork 228
Extract watcher, batcher and monitor into pkg/util #245
Conversation
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.
Looks good 👍 ✨
Just one question I want to hear your opinion on before we go ahead and merge.
) | ||
|
||
// Update bundles an Event with an | ||
// APIType for Storage retrieval. | ||
type Update struct { | ||
Event Event | ||
Event watcher.Event |
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.
Event though watcher
uses the Events the most, they are not watcher
specific, the same events are used to detect changes for Objects in WatchStorage
and SyncStorage
. Should we move Event
to watcher
or keep it in update
, to be generic?
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 think it's better to keep the types fairly closely to where they are used. I did not understand the purpose of the types when seeing them grouped in the update
package. I think they are a good fit in this package, at least until something more significant changes in the codebase.
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.
As it seems to work for WatchStorage
and SyncStorage
in terms of dependencies, LGTM 👍. After a rebase feel free to merge.
Rebased; merging now |
This makes them reusable in a generic manner, and not tied to
pkg/storage