CleverCloud/cliparse-node

stringParser: empty options are passed as boolean true

Closed this issue · 6 comments

In the case of user input error while using options in the form my-executable --myvar1 --myvar2 blue then myvar1 will be set as boolean true, when I think the general case is that empty option strings should return an error. Whereas in the case of flags, of course boolean values are desired.

I'd be happy to submit a pull request, time permitting, with a patch. But I wasn't sure what the ideal solution would be in @divarvel 's mind.

I have a simple workaround for now:

function valid_string_parser(string) {

    if (Boolean(string) === string) {
        return parsers.error('empty strings are not allowed');
    }

    return parsers.success(string.toString());
}

Yep, good catch.
Regarding flags, CLI parse explicitely tells minimist it expects a boolean value, so it's correctly handled.
The correct behaviour would be indeed to return an error if a boolean is given where another value was expected.

If you can submit a patch for this, that would be perfect. Else I'll try to find time.

Do you know where the right place to fix this is? I took a guess and it fixes my concern but it causes 5 tests to fail, so that's likely not the best solution.
master...chrishiestand:missing-option-error

I guess it has to happen inside the parsers themselves, as some parsers (ie the boolean parser) have to accept no value being given (see my commit).
Another solution would be to add a boolean flag in the parser API to let a parser declare if it accepts empty values.

Neither solution please me that much, tbh. The auto-casting behaviour of minimist is a bit of a problem in this case. Regular strings would be much simpler, I'll see what I can do to turn it off.

I'll take a shot with https://github.com/substack/minimist/issues/32 once I'm in my plane.

@chrishiestand I think #20 will improve things, I'll release a version later today, so if you still have the issue, don't hesitate to check it out :-)

Thanks @divarvel. I'll close this issue for now.