-
Notifications
You must be signed in to change notification settings - Fork 13
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
refactor cmd/ & create a standalone client under cmd/client/ #375
Comments
I've started to refactor cmd (#355) handlers (client refactor not included). But cmd pkg needs refactor for sure. For example, accessing all flags via cmd.Flags("flag-name").Vlaue.String method is poor. About cmd handler I really like approach as it's in https://github.com/gohugoio/hugo - hugo was created by spf13 - author of cobra package. Each cmd is an struct and has flags variable inside it (https://github.com/gohugoio/hugo/blob/master/commands/release.go). Also, there are no So I've started this and will push it on Monday, so we can sync this refactoring with the client/api usage. |
@krhubert I like it. Serving each command as a struct gives a good isolation between other commands and prevents naming conflicts on functions. Since commands will never share functionality in between, separating them to their own structs is a good decision. I can take the work for client package. :) |
I gave a quick look on hugo. It doesn't have one but i think it'd be nice to define a Commander interface and make command structs to implement it. related with these lines: 1, 2. type Commander interface {
Command() *cobra.Command
}
var commands *cobra.Command
for commander := range []commanders {
commands = append(commands, commander.Command())
}
cmd.AddCommand(commands...) |
related with #310 |
Check if this one is closed with this PR #444 |
Closing. |
kinda related with https://github.com/mesg-foundation/core/issues/191.
client package
refactor cmd handlers
To make them more logicless and create a CMD type that has handlers for each command.
The text was updated successfully, but these errors were encountered: