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

feat: make positional arguments fully configurable #967

Merged
merged 13 commits into from Oct 12, 2017
Merged

Conversation

bcoe
Copy link
Member

@bcoe bcoe commented Oct 5, 2017

Introduces the .positional() method, which makes positional arguments fully configurable (but wait there's more, positional arguments will also be included separately in help output):

screen shot 2017-10-05 at 12 08 46 am

TODO:

  • make it so we default the settings demanded and array based on the command string.
  • accept the argument $0 to represent a default command (so that it's more intuitive that commands can be used to handle top-level operations).
  • add documentation.
  • get translations for Positionals: (help wanted!). we can do this post-hoc.
  • finish adding tests.

see: #541, #570, #959, etc

cc: @tkarls, @hax, @sloanlance, @iarna, etc.

lib/command.js Outdated
@@ -239,64 +246,73 @@ module.exports = function command (yargs, usage, validation) {
argv._ = argv._.slice(context.commands.length) // nuke the current commands
const demanded = commandHandler.demanded.slice(0)
const optional = commandHandler.optional.slice(0)
const parseOptions = {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

perhaps refactor the population of these parser options into a separate helper based on the command string, since they're also required for .positional().

@bcoe bcoe requested a review from Morishiri October 10, 2017 06:20
@bcoe bcoe changed the title [WIP] feat: make positional arguments fully configurable feat: make positional arguments fully configurable Oct 10, 2017
@bcoe
Copy link
Member Author

bcoe commented Oct 10, 2017

@jeffijoe @ruimarinho @tkarls @atian25 @popomore @matatk @sloanlance

I just took on this major refactor of how we parse commands in yargs to try to:

  1. make commands first class citizens that have most of the same parsing hints as flag options.
  2. make the command API a bit less confusing (by improving documentation, and adding a logical command for configuring commands .positional()).

I would love your feedback -- I'm feeling pretty excited about this update to commands.

@jeffijoe
Copy link

@bcoe very nice, great work!

Copy link
Contributor

@Morishiri Morishiri left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice work :)

@@ -4,3 +4,4 @@ node_modules/
*.swp
test.js
coverage
package-lock.json
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

😎

@bcoe bcoe merged commit cb16460 into master Oct 12, 2017
@bcoe bcoe deleted the configure-positional branch October 12, 2017 03:19
@bcoe
Copy link
Member Author

bcoe commented Oct 13, 2017

@popomore, @hax, @Morishiri, @zkat, @jeffijoe this work is now available on the candidate release of yargs@10.0.0; I would love some help testing (since this is one of the bigger overhauls to yargs' functionality in a while).

You can play with it by running:

npm i yargs@next

@matatk
Copy link

matatk commented Oct 14, 2017

Thanks for working on this; it is looking very promising. I have two questions...

  1. It seems that I cannot declare a positional argument without declaring a command. The following is invalid code. Is there a way to have positional arguments without having to declare a command?

    const argv = require('yargs')
    	.usage('$0 greeting')
    	.positional('greeting', {
    		describe: 'what you would like the program to say',
    		type: 'string'
    	})
    	.help()
    	.argv
    
    console.log(argv)
  2. When argv gets returned, it comes back as an object such as { _: [ 'moo' ], version: false, help: false, '$0': 'index.js' }. When positional arguments are in use, will we be able to use their name, rather than just _ to refer to them? Being able to name the arguments would make code that uses yargs more readable. (There's an original issue along these lines that brought me here, and this was the root of it.)

@bcoe
Copy link
Member Author

bcoe commented Oct 14, 2017

@matatk try this on the most recent branch (npm yargs@next):

const argv = require('./')
  .command('$0 [port]', 'start the server', (yargs) => {
    yargs
      .positional('port', {
        describe: 'port to bind on',
        default: 5000
      })
   })
   .argv

I could imagine an alias for this that looks like this:

const argv = require('./')
  .usage('$0 [port]', (yargs) => {
    yargs
      .positional('port', {
        describe: 'port to bind on',
        default: 5000
      })
   })
   .argv

I wonder if this might be overkill though -- my feeling is it's probably better to encourage people to move towards using command('$0') rather than .usage() when they're describing the properties of positional arguments.

@matatk
Copy link

matatk commented Oct 15, 2017

@bcoe that's brill; just what I was after—super-easy to configure positional arguments for the implicit default command and I get to refer to the positional arguments by name; thanks :-).

@iarna
Copy link
Contributor

iarna commented Oct 15, 2017

@bcoe This looks great! Just be sure to xref in the docs for .usage() and I think it's fine to keep them apart. =)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants