Conversation
| Status(string) ([]persistentqueue.StatusItem, error) | ||
| } | ||
|
|
||
| type Heartbeat interface { |
There was a problem hiding this comment.
We can probably clean this up a bit by moving this interface into heartbeat.go and having the constructor return the interface like: func NewHeartbeatTask() Heartbeat { ... }
There was a problem hiding this comment.
I've been seeing this "accept interfaces, return structs" thing in different posts so I moved the interface to heartbeat.go and then changed the HeartbeatTask struct to heartbeat. Let me know if I'm complete misunderstanding what you're saying, but here's my attempt at addressing the feedback: d615a10
There was a problem hiding this comment.
Ah I see this as a case of generality:
If a type exists only to implement an interface and will never have exported methods beyond that interface, there is no need to export the type itself. Exporting just the interface makes it clear the value has no interesting behavior beyond what is described in the interface.
ChezCrawford
left a comment
There was a problem hiding this comment.
I think this looks good for an initial implementation.
As I was completing this review I realized that it would probably make sense for us to use RetryTransport here since it already has solid HTTP retry logic built into it. Rather than delaying this we can cut a separate ticket to come back and replace this.
There was a problem hiding this comment.
This looks good @rafusel.
Before you merge:
-
have a look at one tiny PR I put up into this branch: https://github.com/PagerDuty/go-pdagent/pull/18/files that is a small refactor to the heartbeat HTTP call. I left a few comments about why I think it is slightly cleaner.
-
Squash everything down to a single commit.
No description provided.