-
Notifications
You must be signed in to change notification settings - Fork 19
Organize commands #251
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
Organize commands #251
Conversation
25fbf9f to
16a9f56
Compare
|
If republish and update are not implemented, delete them. |
|
@ajnavarro there are PRs implementing that functionality #250 #249, just left the structure to make easier the integration with that code. Should I remove them anyway? |
|
Yes, I would prefer to add that structure on the other PRs with the actual functionality. |
cli/borges/command.go
Outdated
| Execute(args []string) error | ||
| } | ||
|
|
||
| type Commander interface { |
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.
Why commander and not just Command?
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 saw in the go-flags library that name too, but yes, Command and ExecutableCommand looks better. Will change that.
| return nil | ||
| } | ||
|
|
||
| func init() { |
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'd prefer initializing the commands in the main as it was before instead of having the initializations scattered across all files and sharing a global parser, what do you think?
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 that each command adds itself and don't have an enormous main but whatever all of you agree it's ok for me.
cli/borges/file.go
Outdated
|
|
||
| const ( | ||
| fileCmdName = "file" | ||
| fileCmdShortName = "produce jobs from file" |
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.
Shouldn't be fileCmdShortDesc?
cli/borges/mentions.go
Outdated
|
|
||
| const ( | ||
| mentionsCmdName = "mentions" | ||
| mentionsCmdShortName = "produce jobs from mentions" |
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.
mentionsCmdShortDesc?
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.
indeed, changing it..
7d150e6 to
b5fe323
Compare
Signed-off-by: Manuel Carmona <manu.carmona90@gmail.com>
Signed-off-by: Manuel Carmona <manu.carmona90@gmail.com>
Signed-off-by: Manuel Carmona <manu.carmona90@gmail.com>
…mmand Signed-off-by: Manuel Carmona <manu.carmona90@gmail.com>
b5fe323 to
d47ce6e
Compare
This it the borges command organization now:
The producer command has been split in several subcommands:
About the code, each command is in its own file and it's added to the global parser by the
func init()in that file, making easier aggregate more commands.The subcommands are in their own files too, but the addition is handled in the
func init()of the command parent.The functionality of the commands
republishandupdatearen't implemented in this PR.Closes #244