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

How to stop parsing for options after first argument? #1127

Closed
karfau opened this issue Dec 28, 2019 · 16 comments
Closed

How to stop parsing for options after first argument? #1127

karfau opened this issue Dec 28, 2019 · 16 comments

Comments

@karfau
Copy link
Contributor

karfau commented Dec 28, 2019

I have the following usage pattern:

runex [options] <module> [args...]
where args should be everything that is listed after module, even if it matches any option I have configured using .option().

Is there a way to tell commander to stop on the first positional argument it found and pass the rest to args?
Or a way to hook into commander event listeners to achieve this?
(A related question of understanding: are arguments only supported on commands or also on the root level?)

I tried the following code:

const runnable$ = '<runnable> [args...]';
new Command('runex').usage(`[options] ${runnable$}`)
  .option(
    '-r, --require <module>', '0..n modules for node to require', collectDistinct, []
  )
  .arguments(runnable$)
  .parse(process.argv]

but the following case is not working as expected:

runex -r ts-node/register path/to/script -r

since it exits with

error: option '-r, --require <module>' argument missing

I'm not sure but #193 might be related.

I'm happy to support adding this as a new option.

@shadowspawn
Copy link
Collaborator

Is there a way to tell commander to stop on the first positional argument it found and pass the rest to args?

No, but you can use -- to tell Commander to stop looking for options and treat the remaining arguments as operands. e.g.

runex -r ts-node/register path/to/script -- -r

https://pubs.opengroup.org/onlinepubs/9699919799/basedefs/V1_chap12.html#tag_12_02

Guideline 10:
The first -- argument that is not an option-argument should be accepted as a delimiter indicating the end of options. Any following arguments should be treated as operands, even if they begin with the '-' character.

@shadowspawn
Copy link
Collaborator

(A related question of understanding: are arguments only supported on commands or also on the root level?)

Both. You are actually adding arguments to the root level command in your example!

See https://github.com/tj/commander.js#specify-the-argument-syntax

You use .arguments to specify the arguments for the top-level command, and for subcommands they are included in the .command call.

@shadowspawn
Copy link
Collaborator

I'm happy to support adding this as a new option.

Positional option parsing does come up sometimes in issues, and I am interested, if you want to look into it. I suspect it may be tricky to add!

@karfau
Copy link
Contributor Author

karfau commented Dec 30, 2019

In the mean time I switched to yargs-parser that offers that option. And since my lib is still small regarding options, I'm handwriting the usage part for now.
Feel free to close this issue.

@shadowspawn
Copy link
Collaborator

shadowspawn commented Dec 30, 2019

Related previous issue: #1033

Closing as details provided and OP no longer interested.

@karfau
Copy link
Contributor Author

karfau commented Dec 31, 2019

Actually I'm still not convinced by my move away from commander. I like it's "fail early" approach more than the flexibility others offer.

Where/How would you suggest to add this feature?
Do you think it should be implemented as some (opt-in) option, so it is not a breaking change?
I will start to have a look at the code base, to get some ideas, but any hint is welcome.

@shadowspawn shadowspawn reopened this Jan 1, 2020
@shadowspawn
Copy link
Collaborator

shadowspawn commented Jan 1, 2020

Where/How would you suggest to add this feature?

parseOptions removes the options it recognises and splits the remaining arguments into "known" and "unknown". This is where you could stop processing options when find an "operand" (i.e. something which is not an option or an option value).

(The approach of splitting the arguments is flawed because it can change the argument order, and I am hoping to eventually have a go at rewriting the logic... Edit: working on it now...)

Note there is special processing for -- and everything afterwards gets thrown into "known". I think this new mode would be somewhat the opposite, all the options after the first operand would get thrown into "unknown" for later processing.

Do you think it should be implemented as some (opt-in) option, so it is not a breaking change?

Yes. Backwards compatibility is important.

My first thought is most people will either want (all) global options, or (all) positional options. I'm thinking there is a new call to switch the default behaviour (rather than control per option).

e.g. .makeOptionsPositional

I will start to have a look at the code base, to get some ideas, but any hint is welcome.

Good luck. I'm busy elsewhere in the code so won't be doing a deep dive at the moment, but ask if you have questions I might be able to help with in the meantime.

@shadowspawn
Copy link
Collaborator

Opened PR for rewrite of .parseOptions to fix argument ordering issues: #1138

@karfau
Copy link
Contributor Author

karfau commented Jan 7, 2020

If I understand your last message correctly, it will be much easier to add the feature I requested in v5 / when that PR landed, right?

(Sorry for the above redundant "noise", was just a rebase of a branch. I removed the link from the commit message now.)

@shadowspawn
Copy link
Collaborator

A little easier, and a lot less likely to get a merge conflict. :-)

@shadowspawn
Copy link
Collaborator

shadowspawn commented Jan 10, 2020

Second PR open now too: #1145
I am not working on any other havoc to .parseOptions()!

@shadowspawn
Copy link
Collaborator

Found another related issue for positional options: #797

@shadowspawn
Copy link
Collaborator

No activity in a month. I did some research and rethinking the higher level goals. Closing in favour of #1229.

@shadowspawn
Copy link
Collaborator

Pull Request opened to add .enablePositionalOptions() and .passThroughOptions(): #1427

@karfau
Copy link
Contributor Author

karfau commented Jan 4, 2021

Sorry, currently I'm having a hard time following all the threads and descriptions to solve my requirement.
Are you able to answer my initial question with relation to the features available now?

Thx

@shadowspawn
Copy link
Collaborator

shadowspawn commented Jan 4, 2021

Just add .passThroughOptions():

const runnable$ = '<runnable> [args...]';
new Command('runex').usage(`[options] ${runnable$}`)
  .passThroughOptions() // <------
  .option(
    '-r, --require <module>', '0..n modules for node to require', collectDistinct, []
  )
  .arguments(runnable$)
  .action((runnable, args, options, command) => {
    console.log(`runex: ${runnable} ${args}`);
  })
  .parse(process.argv)
% node index.js -r ts-node/register path/to/script -r 
runex: path/to/script -r

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

2 participants