Skip to content
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

Better error message on unrecognized command #1468

Merged
merged 1 commit into from
Jul 28, 2015

Conversation

sbruce
Copy link
Contributor

@sbruce sbruce commented Jul 13, 2015

License: MIT
Signed-off-by: Shaun Bruce [email protected]

@jbenet jbenet added the backlog label Jul 13, 2015
}
return sFinal
}

// if we have more arg values provided than argument definitions,
// and the last arg definition is not variadic (or there are no definitions), return an error
notVariadic := len(argDefs) == 0 || !argDefs[len(argDefs)-1].Variadic
if notVariadic && len(inputs) > len(argDefs) {
Copy link
Member

Choose a reason for hiding this comment

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

I wonder if there are any legitimate cases where we would want to print this.. Or if this always means 'you spelled it wrong'.

@sbruce sbruce force-pushed the better-cli-parser-errors branch 2 times, most recently from 9941dc5 to fdd97db Compare July 13, 2015 19:24
@sbruce
Copy link
Contributor Author

sbruce commented Jul 13, 2015

Thanks for the feedback. I moved the code to a new file. I also renamed the searchUknownCmd func to suggestUnknownCmd. That name sounded a little better.

"strings"

cmds "github.com/ipfs/go-ipfs/commands"
levenshtein "github.com/texttheater/golang-levenshtein/levenshtein"
Copy link
Member

Choose a reason for hiding this comment

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

will want to vendor. run

make vendor

from root.


in the event that godeps barfs at you with all sorts of requests, try go get on all the needed packages.

people report that this works too:

godep restore

or even setting

export GOPATH=`pwd`/Godeps/_workspace:$GOPATH

@jbenet
Copy link
Member

jbenet commented Jul 14, 2015

as someone liable to type with one hand, i find this nice. I'm curious what other people think though and whether this is stuff to put in ipfs, or to leave up to shells?

@whyrusleeping
Copy link
Member

my golden rule: "what does git do?"

whyrusleeping@idril ~> git stats
git: 'stats' is not a git command. See 'git --help'.

Did you mean this?
    status

@whyrusleeping
Copy link
Member

@sbruce if you want, one of us can probably handle the godep vendoring. Its quite the obnoxious process

@sbruce
Copy link
Contributor Author

sbruce commented Jul 14, 2015

@whyrusleeping Thanks, but I think I finally figured out godeps.

@jbenet
Copy link
Member

jbenet commented Jul 14, 2015

@sbruce almost there. need to

  • run make vendor (which runs: godep save -r ./... and rewrites the import paths to point to our workspace)
  • squash the commits in one, so that tests pass on all commits even if one is offline.

@sbruce sbruce force-pushed the better-cli-parser-errors branch 4 times, most recently from fbdf39a to d5496c1 Compare July 15, 2015 11:13
@sbruce
Copy link
Contributor Author

sbruce commented Jul 15, 2015

Okay, make vendor should be fixed. I also squashed, and rebased with master.

@whyrusleeping
Copy link
Member

LGTM

@jbenet
Copy link
Member

jbenet commented Jul 16, 2015

@sbruce i should've brought this up. could you write a test for it? if people start depending on this, i want to make sure it works. a sharness test would do. this should get you started:

cat <the-below> >test/sharness/t0150-clisuggest.sh
chmod +x test/sharness/t0150-clisuggest.sh
#!/bin/sh

test_description="Test ipfs cli cmd suggest"

. lib/test-lib.sh

test_suggest() {


    test_expect_success "test command fails" '
        test_must_fail ipfs <cmd> >actual
    '

    test_expect_success "test one command is suggested" '
        grep "<cmd1>" actual ||
        fsh cat actual
    '

    test_expect_success "test command fails" '
        test_must_fail ipfs <cmd> >actual
    '

    test_expect_success "test multiple commands are suggested" '
        grep "<cmd1>" actual &&
        grep "<cmd2>" actual ||
        fsh cat actual
    '

}

test_init_ipfs

test_suggest

test_launch_ipfs_daemon

test_suggest

test_kill_ipfs_daemon

test_done

@sbruce sbruce force-pushed the better-cli-parser-errors branch 2 times, most recently from 5c0d7f3 to f4cf3b8 Compare July 17, 2015 15:08
@sbruce
Copy link
Contributor Author

sbruce commented Jul 17, 2015

A sharness test has been added. Let me know if you think it isn't thorough enough.

@jbenet
Copy link
Member

jbenet commented Jul 28, 2015

@sbruce sorry for the delay. I missed the test update. this LGTM. could you rebase on master? i'd be happy to include it in 0.3.6 if it's rebased today (else 0.3.7).

@jbenet
Copy link
Member

jbenet commented Jul 28, 2015

and thanks! 👍 😄

Closes issue ipfs#1436

License: MIT
Signed-off-by: Shaun Bruce <[email protected]>
@sbruce
Copy link
Contributor Author

sbruce commented Jul 28, 2015

I just rebased to master and pushed the branch. It should hopefully be ready to go.

And, you're welcome. I'm looking forward to helping out more.

@jbenet
Copy link
Member

jbenet commented Jul 28, 2015

thanks @sbruce ! 👍

jbenet added a commit that referenced this pull request Jul 28, 2015
Better error message on unrecognized command
@jbenet jbenet merged commit 4cce2e6 into ipfs:master Jul 28, 2015
@jbenet jbenet removed the backlog label Jul 28, 2015
@jbenet jbenet added this to the IPFS 0.3.6 milestone Jul 28, 2015
@ajnavarro ajnavarro mentioned this pull request Aug 24, 2022
72 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants