-
Notifications
You must be signed in to change notification settings - Fork 57
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
feat(cli): enable extended client access #285
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.
Just a comment about the name.
I'd be fine with this. However, are you able to link to the previous discussion where I pushed back on the whole concept of the clientMixin
? I can't find that right now, and I'm interesting in going back over my thoughts.
clientSetter
as ClientInitializer
.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 prefer the new naming.
This will allow commands to get a Pebble client instance through the command struct constructor.
66db1f6
to
c28ec56
Compare
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 great. Few super nitty questions / comments.
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.
Great job !
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.
Yeah, I like this -- it's a tiny bit more verbose, but it seems much more direct and Go-like than the "mixin" concept.
f9f8380
to
4b90695
Compare
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.
This turned out very well, thank you. Only minor comments below and LGTM with these considered.
Discussed final state with Gustavo and got go-ahead to merge. |
This patch gets rid of the
clientMixin
in command structs in favor of passing aCmdOptions
struct to theNew
(formerlyBuilder
) function inCmdInfo
.Example usage:
Command implementation
Client implementation