-
Notifications
You must be signed in to change notification settings - Fork 108
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
Add /api/consumers support #140
Add /api/consumers support #140
Conversation
consumers.go
Outdated
func (c *Client) ListConsumers() (rec []ConsumerInfo, err error) { | ||
req, err := newGETRequest(c, "consumers") | ||
if err != nil { | ||
return []ConsumerInfo{}, err |
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.
You should be able to return nil, err
which should be preferred as you wouldn't be allocating when returning an error.
Since you've named your return values you should even be able to just return
.
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 addressed those issues in 13d99fc
.
"net/url" | ||
) | ||
|
||
type BriefQueueInfo struct { |
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.
Thank you for inferring and following the convention :)
consumers.go
Outdated
|
||
type ConsumerInfo struct { | ||
Arguments map[string]interface{} `json:"arguments"` | ||
AckRequired bool `json:"ack_required"` |
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.
Ideally we should introduce an AcknowledgementMode
enum type which has two values, Manual
(acks are expected) and Automatic
(fire-and-forget). This would be clearer than a boolean with this name. We could also introduce additional functions on ConsumerInfo
. WDYT?
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 can try to implement the new type I don't have anything against it, I only did it that way so that the new types match what's returned by the API.
Regarding new functions on ConsumerInfo
what new functions would you like to see ?
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 added AcknowledgementMode
in 641510a
let me know if it is what you were looking for.
Thank you. This looks reasonable. I have a naming/type suggestion around consumer acknowledgment mode. |
Thank you! |
This pull request add support for:
I added some tests but I'm not that familiars with gomega, so please let me know of anything seems wrong to you.
All the tests are passing.