Skip to content
This repository has been archived by the owner on Jul 15, 2018. It is now read-only.

use spf13/cobra instead of urfave/cli #117

Merged
merged 12 commits into from
Oct 27, 2017
Merged

use spf13/cobra instead of urfave/cli #117

merged 12 commits into from
Oct 27, 2017

Conversation

zramsay
Copy link
Contributor

@zramsay zramsay commented Oct 18, 2017

@zramsay zramsay changed the title WIP: use spf13/cobra instead of urfave/cli use spf13/cobra instead of urfave/cli Oct 18, 2017
Use: "console",
Short: "Start an interactive abci console for multiple commands",
Long: "",
ValidArgs: []string{"batch", "echo", "info", "set_option", "deliver_tx", "check_tx", "commit", "query"},
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor

@ebuchman ebuchman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is great other than the exit strategy. We should only use RunE and always return an error.

bufReader := bufio.NewReader(os.Stdin)
for {
line, more, err := bufReader.ReadLine()
if more {
return errors.New("Input line is too long")
ifExit(errors.New("Input line is too long"))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should only use RunE and return errors here.

args := persistentArgs(line)
app.Run(args) //cli prints error within its func call
pArgs := persistentArgs(line)
out, err := exec.Command(pArgs[0], pArgs[1:]...).Output()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

shelling it out? would be better if we could keep in process

if len(args) != 1 {
return errors.New("Command check_tx takes 1 argument")
ifExit(errors.New("Command check_tx takes only 1 argument"))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no need for "only"

srv, err := server.NewServer(addrC, abci, app)
if err != nil {
logger.Error(err.Error())
os.Exit(1)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

return err

@@ -426,3 +499,10 @@ func stringOrHexToBytes(s string) ([]byte, error) {

return []byte(s[1 : len(s)-1]), nil
}

func ifExit(err error) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

remove this and all use of os.Exit in functions

@ebuchman ebuchman merged commit 66de532 into develop Oct 27, 2017
@ebuchman ebuchman deleted the cobra-not-urfave branch October 27, 2017 06:47
odeke-em added a commit that referenced this pull request Dec 10, 2017
Use the single client connection at startup time
for sending over commands instead of shelling out
for every command.
This code fixes the regression from
#117
which instead used "os/exec".Command with:
    "abci-cli <the_command> [args...]"

The purpose of this code is to restore us
back to the state after cobra replace urlfave/cli.
There is still a bit of work to implement Batch
itself, but that should be simpler as a focused
command.

Fixes #133
odeke-em added a commit that referenced this pull request Dec 16, 2017
Use the single client connection at startup time
for sending over commands instead of shelling out
for every command.
This code fixes the regression from
#117
which instead used "os/exec".Command with:
    "abci-cli <the_command> [args...]"

The purpose of this code is to restore us
back to the state after cobra replace urlfave/cli.
There is still a bit of work to implement Batch
itself, but that should be simpler as a focused
command.

Fixes #133
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants