Skip to content

[New] if a demand message is provided, gsub in “got” and “count”#438

Closed
ljharb wants to merge 3 commits into
yargs:masterfrom
ljharb:demand_message_replacements
Closed

[New] if a demand message is provided, gsub in “got” and “count”#438
ljharb wants to merge 3 commits into
yargs:masterfrom
ljharb:demand_message_replacements

Conversation

@ljharb

@ljharb ljharb commented Mar 14, 2016

Copy link
Copy Markdown
Contributor

thar she blows, fixing #431!

@bcoe

bcoe commented Mar 14, 2016

Copy link
Copy Markdown
Member

will land this early this week.

@bcoe

bcoe commented Mar 15, 2016

Copy link
Copy Markdown
Member

@lrlna @nexdrew thoughts?

@nexdrew

nexdrew commented Mar 15, 2016

Copy link
Copy Markdown
Member

You can kinda already do this:

require('yargs')
  .updateLocale({
    'Not enough non-option arguments: got %s, need at least %s': 'got %s, expected %s'
  })
  .demand(1)
  .argv
$ node issue431.js

got 0, expected 1

@ljharb

ljharb commented Mar 15, 2016

Copy link
Copy Markdown
Contributor Author

@nexdrew good to know, but that's a very obscure and undocumented way of doing something relatively straightforward :-)

@lrlna

lrlna commented Mar 15, 2016

Copy link
Copy Markdown
Member

o.O oh cool, didn't know that @nexdrew

We also already have demand to output the same kind of deal when no argument other than num is provided.

require("./")
  .command("config", "set up this module")
  .command("send", "send a message")
  .command("start", "start a conversation")
  .demand(2)
  .argv
$ node issue431.js 

Commands:
  config  set up this module
  send    send a message
  start   start a conversation

Not enough non-option arguments: got 0, need at least 2

I guess this is for ✨ ultra fancy ✨ messages folks might want ?

@nexdrew

nexdrew commented Mar 15, 2016

Copy link
Copy Markdown
Member

@ljharb Agreed. It's not entirely undocumented (see here), but it is obscure because you basically have to look in the locale files of yargs source code to know what your options are. The cool thing about it is that you can override just about any yargs-defined string this way, but it does have limitations and is arguably not straightforward.

That being said, I'm not opposed to this pull. I'd like to avoid doing the same thing in a bunch of different ways, but this change (1) seems simple and straightforward, (2) is somewhat orthogonal to i18n, and (3) makes for a nicer API. So looks good to me! 👍

@ljharb

ljharb commented Mar 15, 2016

Copy link
Copy Markdown
Contributor Author

@lrlna yes, exactly - i wanted to make a message that described them better than "non-option arguments", but still include the actual vs expected counts.

@lrlna

lrlna commented Mar 15, 2016

Copy link
Copy Markdown
Member

@ljharb I am all for it 👍

@ljharb

ljharb commented Mar 16, 2016

Copy link
Copy Markdown
Contributor Author

@bcoe friendly ping! it's halfway through the week :-)

@bcoe

bcoe commented Mar 17, 2016

Copy link
Copy Markdown
Member

@ljharb (CC: @lrlna) working on code reviewing this, an edge-case popped up.

What should we do if you are demanding arguments, or arguments and positional arguments?

#!/usr/bin/env node
var argv = require('./')
  .demand(['a', 'b'], "pork chop sandwhiches $0")
  .argv;

console.log(argv)

or

#!/usr/bin/env node
var argv = require('./')
  .demand(1, ['a', 'b'], "pork chop sandwhiches $0")
  .argv;

console.log(argv)

ends up outputting:

Missing required arguments: a, b
pork chop sandwhiches $0

Is there something else we should be outputting? Perhaps this is just an edge-case, i.e., $0 ... $x is only replaced for positional arguments?

@ljharb

ljharb commented Mar 17, 2016

Copy link
Copy Markdown
Contributor Author

@bcoe good question. I don't think it would be sensible to output counts in that case, and outputting the array of received arguments isn't as useful, because the user already knows which explicit named args they provided. (in other words, the usefulness as i see it of the counts in the message is because it can be easy to miscount non-option args when typing them)

@elas7

elas7 commented Mar 17, 2016

Copy link
Copy Markdown
Member

I think there's another edge case.

There are 2 messages that can be displayed when the count is invalid:

  • Not enough non-option arguments: got %s, need at least %s
  • Too many non-option arguments: got %s, maximum of %s

So, if a custom message is provided, it won't make too much sense in one of those cases.

A solution may be to find a way to provide two custom error messages. Or to provide $2 as demanded._.max to the custom message, so as to be able to create a message that handles both cases. Or to use @nexdrew method of calling updateLocale()

@ljharb

ljharb commented Mar 17, 2016

Copy link
Copy Markdown
Contributor Author

aha, i didn't realize there was a "max" message. That means that even the current API of accepting one message doesn't make sense.

Would you be amenable if I updated the PR so .demand(count[, notEnoughMessage[, tooManyMessage]]) was the signature?

@bcoe

bcoe commented Mar 17, 2016

Copy link
Copy Markdown
Member

@ljharb it seems like we've uncovered a bit of a hornet's nest. I've got a busy day at the npm mines today, but I'm going to reflect on this problem -- In a perfect world we'd make providing a custom message a bit easier, without breaking the existing API.

@ljharb

ljharb commented Mar 20, 2016

Copy link
Copy Markdown
Contributor Author

@bcoe what would you think about the .demand(count[, notEnoughMessage[, tooManyMessage]]) signature, but using updateLocale internally? That way the %s would be consistent, and it would just be sugar for the less intuitive approach of updateLocale?

@coveralls

Copy link
Copy Markdown

Coverage Status

Coverage remained the same at 100.0% when pulling d7eba52 on ljharb:demand_message_replacements into 0e3b9a6 on yargs:master.

@coveralls

Copy link
Copy Markdown

Coverage Status

Coverage remained the same at 100.0% when pulling ebf733a on ljharb:demand_message_replacements into 0e3b9a6 on yargs:master.

@ljharb

ljharb commented Mar 20, 2016

Copy link
Copy Markdown
Contributor Author

I've updated the PR to include a message override param in demand for "too many".

At the moment it still uses the $0/$1 notation, but if indicated, I'll update it to use the translation internals accordingly.

I'll also add any missing test coverage once a direction is settled.

@coveralls

Copy link
Copy Markdown

Coverage Status

Coverage decreased (-0.1%) to 99.888% when pulling 7db3dde on ljharb:demand_message_replacements into 0e3b9a6 on yargs:master.

@bcoe

bcoe commented Mar 23, 2016

Copy link
Copy Markdown
Member

@ljharb @nexdrew okay, demand overriding is a tire fire is my conclusion ... I started putting some work into merging this today -- and realized another edge-case (which is why the build is currently failing with test-coverage issues). We need to cover this case:

.demand(['arg', 'arg'], 1, 2, 'min message', 'max message', 'argument message?')

I propose that rather than adding the new maxMessage argument (which complicates things further) I propose that we deprecate .demand(), and introduce two new methods: demandCommand, demandOption.

.demandCommand(min, max, minMsg, maxMsg)
.demandCommand(min, minMsg)
.demandOption('option', 'msg')
.demandOption(['option', 'option'], 'msg')

I was against this at first, but at the end of the day I think splitting out the logic like this will start to put out the tire fire ... perhaps we can even refactor .demand() to call demandCommand and demandOption.

@ljharb

ljharb commented Mar 24, 2016

Copy link
Copy Markdown
Contributor Author

Sounds good - I'll work on a PR to add the two new methods.

@bcoe

bcoe commented Apr 10, 2016

Copy link
Copy Markdown
Member

@ljharb would love to get this polished, happy for some other folks in the yargs org to take on the work 👍 but also happy to wait for your contribution.

@ljharb

ljharb commented Apr 10, 2016

Copy link
Copy Markdown
Contributor Author

Sorry about the delay, time's gotten away from me :-) if it's done in the meantime that's OK, otherwise I'll try to get something up soon.

@bcoe

bcoe commented Apr 11, 2016

Copy link
Copy Markdown
Member

@ljharb nothing to apologize for, I appreciate the contribution so far. if anyone else contributes to this feature, we should make sure it's made as a pull-request to your branch so that you can coordinate with them.

@bcoe

bcoe commented May 1, 2016

Copy link
Copy Markdown
Member

@maxrimue @lrlna, etc, does anyone else want to help see this over the finish line? we went down a rabbit hole with the function signature of demand.

@lrlna

lrlna commented May 2, 2016

Copy link
Copy Markdown
Member

hii! imma look into it ✌️, @bcoe //cc @maxrimue

@bcoe

bcoe commented May 14, 2016

Copy link
Copy Markdown
Member

@lrlna @ljharb bumping this pull; if we don't have any time to think about how we should fix this historic API right now, perhaps we should create a tracking ticket in issues and close this for the time being? Just so we don't have pull requests kicking around for too long :)

@ljharb

ljharb commented May 14, 2016

Copy link
Copy Markdown
Contributor Author

I'm fine with that - as long as something remains open :-)

@lrlna

lrlna commented May 15, 2016

Copy link
Copy Markdown
Member

I've opened an issue to track this and will close this pull request for now. Hope to finish the PR by the end of the day 🎉

@lrlna lrlna closed this May 15, 2016
@ljharb ljharb deleted the demand_message_replacements branch June 8, 2016 03:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants