deno_std icon indicating copy to clipboard operation
deno_std copied to clipboard

feat(cli): add `parse()` with descriptive schema

Open timreichen opened this issue 1 year ago • 9 comments

ref: https://github.com/denoland/deno_std/issues/4272

This PR adds parse() that takes a descriptive schema for parsing args. This is inspired by rusts clap, yargs and commanderjs.

This needs more tests for edge cases and some discussions about features like

  • Option.value.accepted?: T[] for allowed values as a shortcut for `Option.fn: (value) => { if (!accepted.includes(value)) { throw new Error("...")}
  • Option.value.min?: number for a minimum of arguments taken
  • Option.value.max?: number for a maximum of arguments taken

The type generics also need some rework to imply correct types based on Option.default and Option.type for returned result and inside Option.fn(value: T) Feedback and suggestions are very welcome. @iuioiua @ngdangtu-vn

timreichen avatar Feb 20 '24 11:02 timreichen

This is the 2nd set of API for parsing cli arguments. It feels confusing to have 2 different sets of APIs for the same purpose.

I'd recommend you develop this tool as a 3rd party tool. If that tool got popularity and adoption in the ecosystem, we would be able to consider adopting it in std

kt3k avatar Feb 24 '24 13:02 kt3k

This is the 2nd set of API for parsing cli arguments. It feels confusing to have 2 different sets of APIs for the same purpose.

I agree, two wouldn't make too much sense, though the one currently implemented is based on minimist and is not capable of some use cases, especially comparing it to other poplar tools like cliffy, nor compatible with such a schema based approach (as pointed out in https://github.com/denoland/deno_std/issues/4272)

I'd recommend you develop this tool as a 3rd party tool. If that tool got popularity and adoption in the ecosystem, we would be able to consider adopting it in std

There are plenty of popular 3rd party tools out there (commander, yargs, cliffy, etc.) that have this declarative approach implemented and are far more popular than minimist. So I think publishing another 3rd party tool wouldn't make much sense to prove that point.

The declarative approach this implements avoids the function call chains and replaces them with native objects and arrays to make it less framework-ish, but is similar to the named modules. Example:

...
.option("--foo")
.option("--bar")

=>

{ ...
  options: [
    { name: "foo", },
    { name: "bar", },
  ]
}

timreichen avatar Feb 24 '24 15:02 timreichen

Deno is fantastic tool for writing scripts and CLIs in general. I've been using it a lot lately and find that I can get a lot of mileage using just Deno + @std. The only other libraries I typically reach for are dax & cliffy.

For parsing args, the @std parseArgs works in a pinch for very basic usage. However, I don't find the API overly intuitive and quite simplistic for anything but very basic scripts where I don't feel the need to provide a nice DX on top.

The direction @timreichen is taking here would be much more preferred imo then the existing parseArgs. I believe it would help bridge the gap between very simplistic scripts/CLIs and a more advanced CLI which I'd typically reach for cliffy to implement parsing with.

andrewthauer avatar Mar 02 '24 15:03 andrewthauer

There are plenty of popular 3rd party tools out there (commander, yargs, cliffy, etc.) that have this declarative approach implemented and are far more popular than minimist.

How do you measure the popularity of packages? yargs has 72M weekly downloads, and minimist has 44M. I think these are similarly popular.

Also I don't see what declarative approach means. minimist (and therefore parseArgs of std/cli) accepts the entire parameter all at once as an option object. There's nothing procedural here. I would say minimist also has 'declarative' API.

So I think publishing another 3rd party tool wouldn't make much sense to prove that point.

Without being tested as 3rd party library, how can we be convinced that this design is better than minimist?

kt3k avatar Apr 30 '24 04:04 kt3k

Sidenote: What I mean by declarative approach

I mean having all declarations in one object structured in a certain way:

Option properties

While minimist declares options in an object, it is done so by splitting the declaration by property.

property->objectName

parseArgs(Deno.args, {
  boolean: ["color"], // type is declared here
  default: { color: true }, // default value is declared here
  negatable: ["color"], // negatable is declared here
});

commanderjs, yargs and friends do it the other way around, where all properties of an option are gathered in one object (or in commanderjs case in one command).

object->properties

parse(Deno.args, {
  options: [
    { name: "color", type: Boolean, default: true, negatable: true } // all option properties declared in one object
  ]
});

Subcommands

As explored here, minimist does not support subcommand parsing and a possible implementation would lead to messy code due to the nature of property->objectName.

Named values

Since there is no option object but a collection of properties declared, it is not possible to have named values for options. This is a problem, when one wants to be able to print help for a cli.

property->objectName

???

object->properties

parse(Deno.args, {
  options: [
    { name: "foo", value: { name: "VALUE" } }
  ],
});

How do you measure the popularity of packages? yargs has 72M weekly downloads, and minimist has 44M. I think these are similarly popular.

Yes, but commanderjs has 141M weekly downloads. So I would say together with yargs, that this kind of approach is much more popular.

Without being tested as 3rd party library, how can we be convinced that this design is better than minimist?

I was talking about the fact that commanderjs and other libs that follow such an implementation are already used extensively and are more popular than minimist (by weekly downloads). Some functionality like subcommands are apparently not possible with minimist.

timreichen avatar May 01 '24 06:05 timreichen

Since there is no option object but a collection of properties declared, it is not possible to have named values for options. This is a problem, when one wants to be able to print help for a cli.

I don't see this argument very well. --help is usually done like the below with the current parseArgs:

import { parseArgs } from "jsr:@std/cli/parse-args";

const args = parseArgs(Deno.args, {
  boolean: ["help"],
});
// This becomes { _: [], help: true }, { _: [], help: false }, etc.

if (args.help) {
  console.log("Usage");
  Deno.exit(0);
}

Isn't { _: [], help: true } named option?

Also I don't see well what { name: "VALUE" } part does in your example..

I was talking about the fact that commanderjs and other libs that follow such an implementation are already used extensively and are more popular than minimist (by weekly downloads).

But this suggested API design isn't similar to commander nor yargs at all. It's completely an invented design, which is not tested anywhere.

kt3k avatar May 01 '24 09:05 kt3k

@timreichen BTW what do you think about util.parseArgs of Node.js, which is also available in Deno via node:util module

kt3k avatar May 01 '24 11:05 kt3k

@timreichen BTW what do you think about util.parseArgs of Node.js, which is also available in Deno via node:util module

I think it is very bare-bone. But I like how one defines the options as an object with properties.

timreichen avatar May 13 '24 15:05 timreichen

Codecov Report

Attention: Patch coverage is 86.78414% with 30 lines in your changes missing coverage. Please review.

Project coverage is 96.20%. Comparing base (d93b33a) to head (a4a7566).

Files Patch % Lines
cli/parse.ts 86.78% 29 Missing and 1 partial :warning:
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #4362      +/-   ##
==========================================
- Coverage   96.26%   96.20%   -0.06%     
==========================================
  Files         470      471       +1     
  Lines       38243    38470     +227     
  Branches     5546     5612      +66     
==========================================
+ Hits        36813    37011     +198     
- Misses       1389     1417      +28     
- Partials       41       42       +1     

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

codecov[bot] avatar May 13 '24 15:05 codecov[bot]

types.ts file seems unnecessary to me as the types in it are not shared with other scripts. I think that should be merged in parse.ts

Can you also write some short usage guide which showcases 2, 3 typical usages of parse

kt3k avatar Aug 08 '24 14:08 kt3k

I'll be closing this without merge. parseArgs() is one of the most widely used functions in the Standard Library. Swapping it out with a new implementation presents issues centered around disruption (I'm sure they're obvious, so I won't list them). We're currently focusing on Deno 2, including polishing the Standard Library up for it. So we need to be quite selective on major additions.

I think your PR is heading in the right direction, and the design seems superior to the current implementation. However, the case for the addition must be sufficiently strong for the Deno core team to accept. So I strongly recommend porting this implementation to your own package, and making the design, implementation, documentation, and testing as best as possible. Users should feel a tangible benefit from migrating too. It's probably best done accomplished with others. Then, once ready, present it at sometime in the future as a PR.

That all said, we greatly appreciate the thought and effort you've gone through with this, and other, PRs. Thank you very much.

iuioiua avatar Aug 08 '24 15:08 iuioiua