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

hinting similiar commands #151 #152

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Conversation

minhchu
Copy link

@minhchu minhchu commented May 31, 2016

Using leven package to calculate the difference between two strings. And displays similiar commands which have leven point smaller than 4.
@dthree please check if I'm doing wrong.

@dthree
Copy link
Owner

dthree commented Jun 1, 2016

Looks really good!

Could you add in some tests? I would be interested to see the relative results based on different sets of registered commands.

4 seems like a really high edit distance. For example, if you have the commands ls, bb and ck enabled, and entered a, it would probably suggest all of the above.

@minhchu
Copy link
Author

minhchu commented Jun 2, 2016

I've just run gulp build and added a test suite https://gist.github.com/minhchu/4438f898a8a7bc5fcaa5c72218fca2c6
But there are some problems.
When I run only that test suite mocha --grep "hinting command", it passes. But when I run the whole testmocha`, it throws some unexpected errors:

(node) warning: possible EventEmitter memory leak detected. 11 vorpal_ui_keypres
s listeners added. Use emitter.setMaxListeners() to increase limit.

Do you have any idea ?


If you set maximum distance to 4, it still works for your example because leven distance between a and ls is 2 < 4 ...

@AljoschaMeyer
Copy link

AljoschaMeyer commented Jun 2, 2016

Node gives that warning when more than 10 (?) listeners are attached to the same emitter. Try require('events').EventEmitter.defaultMaxListeners = Infinity in the test file to fix this. Documentation

The cleaner solution would be to set the maximum for the particular event emitter objects. I was just lazy and copy-pasted that snippet from here.

Vorpal should probably do this itself, the threshold doesn't make sense for vorpal anyways.

@dthree
Copy link
Owner

dthree commented Jun 2, 2016

Haven't looked far into it, but are you sure we aren't ignoring some problem?

Is this because the test instantiates at least 10 instances of Vorpal?

While developing it I only got this error when I messed something up on the prompt, and had to fix it.


I'm willing to believe the problem is in the repeated use of:

mute();
var fixture = '\n  fo is an invalid command. Maybe you mean:\n\n    foo\n    fooz\n    bar\n';
help.execSync('fo');
unmute();

@minhchu
Copy link
Author

minhchu commented Jun 3, 2016

Let me check again.
I think there is a problem with this approach. For example:
I have a list of commands:

foo:command
foo:anothercommand
foo:awesomecommand

When I type foo, there will be no suggestions.

@dthree
Copy link
Owner

dthree commented Jun 3, 2016

Yeah, that's all considered one word, and those have a huge edit distance from foo.

}
}
}

Copy link
Contributor

Choose a reason for hiding this comment

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

This can be greatly simplified with es6 syntax.

eg (untested!):

if (invalidString !== ''){

    let catchCmd = this.commands.find(cmd => cmd._catch);

    if (!catchCmd){

        let hints = this.commands.reduce((foundHints, cmd) => {
            if (leven(command, cmd._name) < 4) {
                foundHints.push(cmd._name);
            }
            return foundHints;
        }, []);

        if (hints.length){
            return `\n  ${command} is an invalid command. Maybe you mean:\n\n    ${hints.join('\n    ')}`;
        }
    }
}

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.

4 participants