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

Handle boolean array arguments better #11

Closed
Qix- opened this issue Nov 16, 2018 · 16 comments
Closed

Handle boolean array arguments better #11

Qix- opened this issue Nov 16, 2018 · 16 comments

Comments

@Qix-
Copy link
Contributor

Qix- commented Nov 16, 2018

This is justified since Boolean types already get a bit of special treatment.

In the case I want multiple levels of verbosity, for example, the following should work:

// ./my-program -vvvv

const arg = require('arg');

const args = arg({
    '-v': [Boolean]
});

console.log(args);
/*
    {
        _: [],
        '-v': 4
    }
*/

Currently, you have to do -v -v -v -v separately, and in turn you get [true, true, true, true], which isn't exactly elegant.

@pacocoursey
Copy link
Contributor

I have completed this, but I could use a few more use-cases to ensure it serves the correct purpose.

Using your example above, should ./my-program -vvvv -v output '-v': 5?

What if the flag '-vvv' exists? Should the following return case 1 or case 2?

// ./my-program -vvv -vvv

const args = arg({
    '-vvv': Boolean,
    '-v': [Boolean]
});

console.log(args);

/*
    {
        _: [],

        // case 1...
        '-vvv': true,
        '-v': 3

        // or case 2...
        '-vvv': true
    }
*/

Don't want to miss an edge case 😅

@Qix-
Copy link
Contributor Author

Qix- commented Dec 6, 2018

Using your example above, should ./my-program -vvvv -v output '-v': 5?

Yes. :)

What if the flag '-vvv' exists?

It won't; it's one of arg's few opinions. Single-dash options are only single letters - we have #4 open to enforce this as the example snippet you provided should throw an error.

@blakeembrey
Copy link
Contributor

@Qix- Is there a common use-case where making a special case for this is worth it? It'd make it a bit confusing for anyone briefly reading the docs and it's not that hard to do args.x.filter(x => x).length (though obviously more verbose).

In any case, shouldn't type() always be called? I was just reading the code and I'd expect something like -x false to work (in case the default is true in code).

@Qix-
Copy link
Contributor Author

Qix- commented Dec 9, 2018

@blakeembrey Boolean is already a special case in arg. -x false and -x true are not common GNU argument parsing patterns for booleans. In those cases, false and true would become positional arguments (assuming -x is a boolean type).

That also means that if we kept the current functionality, you'll always end up with an array of true's, which doesn't really add much information aside from using .length anyway.

Further, it makes parsing for condensed arguments (-abcd instead of -a -b -c -d) a bit less intuitive if you accept boolean strings.


The only two edge cases are then:

  • Boolean - if the flag is present, then true; otherwise, false
  • [Boolean] - always a Number, always present and 0 < n, that counts the number of occurrences of the flag in the command line.

Otherwise, all types are as you would expect.

@blakeembrey
Copy link
Contributor

blakeembrey commented Dec 9, 2018

@Qix- I see. Thoughts on using an internal boolean symbol instead (e.g. export const POSITIONAL = Symbol('positional') (or even '__POSITIONAL__' for older systems)? I'm just thinking about how this from documentation in the README and TypeScript. In terms of TypeScript, it'd require an overload in either case to override Boolean behaviour when the numeric is introduced so feel free to ping me then.

If you use an internal symbol, would it make sense to just always be a number? I don't think I'd really care about true or false if it can never be false anyway.

@Qix-
Copy link
Contributor Author

Qix- commented Dec 9, 2018

I'm not sure what problem the boolean symbol would solve. Could you explain a bit more?

As well, perhaps I'm missing something, but [Boolean] argument types are always numbers.

@blakeembrey
Copy link
Contributor

@Qix- Just API consistency. Right now things are super easy to understand, and there's no documentation on Boolean really needed (even though it is special). After this, in either case, you need to write documentation on the new special behavior. I just thought it was more consistent to introduce a new behavior instead of a change to special case to existing statements. E.g. this statement is false now:

This means the built-in String, Number, and Boolean type constructors "just work" as type functions.

Whereas just adding a new sentence mentioning positional flags is easy:

arg({
  '-x': arg.POS,
  '-y': [arg.POS]
})

To be honest, I'm don't feel too strongly either way. I just get a little worried when I see implicit over explicit behavior of inputs since it requires people to "know" this is special and changes behavior. Imagine coming onto a new project and always explaining that [Boolean] actually produces a number. But, you know, everything else is true to the statement above.

@rauchg
Copy link
Member

rauchg commented Dec 10, 2018

I agree with @blakeembrey – I'd argue it's elegant to maintain type affinity. The number result is very surprising when the "type definition" is [Boolean]

@Qix-
Copy link
Contributor Author

Qix- commented Dec 10, 2018

The problem right now is that to specify multiple levels of verbosity, for example, you have to add -v -v -v -v (-vvvv does not work). That's not very GNU-like at all.

Can we at least adopt the ability to do -vvvv even though we keep [true, true, true, true]?


FWIW, specially handling booleans is pretty universally constant. That is to say, just about every argument parser in existence that handles non-argument boolean flags has to support them specially. I don't think it's too much to understand that [Boolean] has special handling.

Getting back [true, true, true, true, true] where the true will never be anything other than true is what's less elegant in my opinion. The only meaningful information you'd get from [Boolean] arguments is by using .length.

@pacocoursey
Copy link
Contributor

Agreed that returning a number for [Boolean] flags seems strange. Seems like the natural solution is to return an array of booleans [true, true, true, ...] and let the user call .length on it themselves.

The core of the issue still exists though: repeating single-hyphen flags should push additional true values onto that array.

@blakeembrey
Copy link
Contributor

@Qix- I do agree by the way! How do you feel about the constant instead of overloading the built-in Boolean function? Is there a name for this behavior? It could just be:

arg({
  '-x': arg.BOOL // `arg.COUNT`, `arg.POS`, `arg.SEEN`, `arg.EXISTS`, `arg.<name of behavior>`
})

I just noticed the third argument too:

The previous value for the destination (useful for reduce-like operatons or for supporting -v multiple times, etc.)

This could be supported by providing arg.COUNT which takes over Boolean and does (_, _, i = 0) => i + 1.

@Qix-
Copy link
Contributor Author

Qix- commented Dec 10, 2018

@blakeembrey I don't hate that, though I feel strongly they should be additional rather than first-class (i.e. the parser shouldn't be aware they even exist).

I think I see your point about symbols now:

// issue #11 is now simply these two lines, and the branch to skip
// looking for an argument is dependent upon the type being a function (!!!)
// with the FLAG_SYM symbol.
//
// I make special note of function checks because the following would be
// invalid, but not checking for it would make the behavior inconsistent and erroneous:
//
//        arg({'--verbose': Boolean, '-v': arg.FLAG_TYPE('--verbose')});
//
// In fact, an assertion/throw can happen in the case of:
//
//        (typeof argType !== 'function' && argType[FLAG_SYM])
//
// EDIT: Alternatively, enforce the type checking in `arg.FLAG_TYPE()` since
// that check would happen less often and is probably better anyway.
const FLAG_SYM = Symbol('arg flag argument type');
arg.FLAG_TYPE = fn => { fn[FLAG_SYM] = true; };

// a new PR is opened to add standard helpers that build off of
// the existing argument type handler specification:
arg.COUNT = arg.FLAG_TYPE((_, _, v) => (v || 0) + 1);

arg.ENUM = map => v => {
    if (v in map) return map[v];
    throw new Error(`invalid argument value: ${v}`);
};

// etc...

I think having some standard helpers would be nice to have. I'm not sure how far outside of the 'unix philosophy' this takes the package, though. @rauchg thoughts?

@blakeembrey
Copy link
Contributor

blakeembrey commented Dec 10, 2018

@Qix- That's a good point. In that case, array support is just which might make that case easier:

function of (fn) {
  return (value, argName, prev = []) {
    prev.push(fn(value))
    return prev
  }
}

arg({
  '-x': of(String)
})

@Qix-
Copy link
Contributor Author

Qix- commented Dec 10, 2018

@blakeembrey Right, but that means losing the simplicity of [Type].

Existing code would have to migrate to something like:

const arg = require('arg');

const args = arg({
    '--include': arg.ARRAY(String),
    '-I': '--include'
});

For comparison, the existing spec which is simpler to write looks like:

const args = arg({
    '--include': [String],
    '-I': '--include'
});

@pacocoursey
Copy link
Contributor

If we want to keep the simplicity of [Type] but avoid the redundancy of returning an array that will only ever contain true values, how about faking it by using [Boolean].length?

This correctly specifies that a number will be returned, and looking at the code correctly implicates that it will return the length of the boolean array. It feels like a hack (and you could just type 1 instead of [Boolean].length) but it would work. Thoughts?

// ./my-program -vv -xxx

const args = arg({
    '-v': [Boolean].length,
    '-x': [Boolean]
});

/*
{
    '-v': 2,
    '-x': [true, true, true]
}
*/

@Qix-
Copy link
Contributor Author

Qix- commented Dec 12, 2018

That feels like a hack :/ I'd say -1 on it.

I still think the symbol and helper method is the way to go.

@Qix- Qix- closed this as completed in #27 Dec 27, 2018
Qix- added a commit that referenced this issue Dec 27, 2018
* add shortarg condensation and arg.COUNT (closes #11)

* update readme with condensed shortarg/arg.COUNT examples

* add example specifically for arg.COUNT

* add documentation for arg.flag()
nevilm-lt pushed a commit to nevilm-lt/arg that referenced this issue Apr 22, 2022
* add shortarg condensation and arg.COUNT (closes vercel#11)

* update readme with condensed shortarg/arg.COUNT examples

* add example specifically for arg.COUNT

* add documentation for arg.flag()
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants