bug(flags): collect is prioritised over boolean

[timreichen]

Describe the bug
Not sure if intentional or a bug: if boolean and collect is set for a flag, it will return [true]. boolean should probably be higher prioritised than collect. Else it should be documented.

Steps to Reproduce

import { parse } from "https://deno.land/std@0.143.0/flags/mod.ts";
const args = parse(["--reload"], { boolean: ["reload"], collect: ["reload"] });

parse returns { _:[], reload: [true] }

Expected behavior
parse should probably return { _:[], reload: true }

Environment

• OS: MacOS 12.4
• deno version: 1.22.0
• std version: 0.143.0

[sigmaSd]

it takes this path https://github.com/denoland/deno_std/blob/main/flags/mod.ts#L340 setArg calls setkey which has collect=true as default

[kt3k]

If you specify arg name in collect, that arg is always parsed as array. So the above behavior is intentional.

options.collect - ... All values will be collected into an array.

The above description should explain it. If you find the docs misleading/not clear, suggestions to the docs are welcome

[timreichen]

@kt3k But what is the point of collecting a boolean flag which always returns [true]?

[kt3k]

You can specify the same arg multiple times, like command -vvv, and you'll get different number of items in the array

import { parse } from "https://deno.land/std@0.143.0/flags/mod.ts";
const a = parse(["-vvv"], { boolean: "verbose", collect: "verbose", alias: { v: "verbose" } });
console.log(a);
// => { _: [], v: [ true, true, true ], verbose: [ true, true, true ] }

[timreichen]

Is this a real use case? I have not encountered anything like that before. Looks more like it should throw an error since alias is not one char long.

[kt3k]

memcached has -v -vv -vvv for different levels of verbosity https://docs.oracle.com/cd/E17952_01/mysql-5.6-en/ha-memcached-cmdline-options.html

symfony (php tool) also seems having them https://symfony.com/doc/current/console/verbosity.html

[timreichen]

Looks to me like the aliases -vv and -vvv should be declared separately (. With that behavior this is valid:

const args = parse(["-rrr"], { boolean: ["reload"], alias: { reload: "r"} });

as is

const args = parse(["-rrr --reload"], { boolean: ["reload"], alias: { reload: "r"} });

Where as deno run -rrr … and deno run -r --reload … it rightfully throws an error:

error: The argument '--reload[=<CACHE_BLOCKLIST>...]' was provided more than once, but cannot be used multiple times

So imo this is also a bug.

In any case, the flag mod seems overly complicated for what it does with boolean arrays and alias maps etc.
I would suggest to deprecate it in favour of an object oriented flag mod: #2341 (comment)

[kt3k]

In any case, the flag mod seems overly complicated for what it does with boolean arrays and alias maps etc.
I would suggest to deprecate it in favour of an object oriented flag mod:

I'm not in favor of changing flags in that way. There's no trivial way to migrate from existing one to that version. So that will cause huge confusion in the ecosystem.

Also std/flags is based on minimist, which has 20K+ dependent libraries. So the design can be considered very well battle tested. Changing this to completely new one only by a speculative reasoning doesn't look like an option to me.

If we completely migrate to a new design, I think we should rather consider adopting another battle tested library such as yargs, commander, etc. (cac, cliffy/flags also might be good candidates, but they have less usages compared to other ones. util.parseArgs of Node.js also might be a candidate, but it's very new)

[timreichen]

Stale