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

choices and coerce sequence #756

Closed
Alexsey opened this issue Jan 13, 2017 · 14 comments
Closed

choices and coerce sequence #756

Alexsey opened this issue Jan 13, 2017 · 14 comments
Assignees
Labels

Comments

@Alexsey
Copy link

@Alexsey Alexsey commented Jan 13, 2017

This code

const argv = require('yargs')
  .choices('user', ['Goofy', 'Miky'])
  .coerce('user', user => {
    switch (user) {
      case 'Goofy': return {
        firstName: 'Mark',
        lastName: 'Pipe'
      }
      case 'Miky': return {
        firstName: 'Elon',
        lastName: 'Stone'
      }
    }
  })

will fail because choices will run after coerce. But should it be this way? Obvious purpose of choices is to validate user input but not generated value. And it would be wary good if we will have an opportunity to validate user input and then build some useful value base on it

@bcoe
Copy link
Member

@bcoe bcoe commented Jan 20, 2017

@Alexsey definitely an edge-case, but I could certainly see an argument for coerce always being applied last ... at the same time, I could see an argument for the other way.

@nexdrew what are your thought son this one, want to play King Solomon on this one?

@hisabimbola
Copy link

@hisabimbola hisabimbola commented Feb 5, 2017

I am also having this issue now. Can you recommend a workaround for this? @bcoe

@hisabimbola
Copy link

@hisabimbola hisabimbola commented Feb 5, 2017

and I think my issue is a bit different but related to this

.option('logLevel', {
    alias: 'l',
    default: 'info',
    describe: 'Enable and set log level',
    choices: ['info', 'silly', 'verbose', 'warn', 'error'],
     coerce: (level) => {
       log.level = level
    },
  })

Anytime I run this I get this error

Invalid values:
  Argument: logLevel, Given: undefined, Choices: "info", "silly", "verbose", "warn", "error"
@bcoe
Copy link
Member

@bcoe bcoe commented Feb 5, 2017

@hisabimbola hoping to do some issue triaging tomorrow; I'll put my thinking cap on and try to get you an answer 👍

@hisabimbola
Copy link

@hisabimbola hisabimbola commented Feb 5, 2017

thanks for your response @bcoe

I was able to get mine to work now, my issue was that I was not returning any value in the coerce method.

Updating the code to this works for me

.option('logLevel', {
    alias: 'l',
    default: 'info',
    describe: 'Enable and set log level',
    choices: ['info', 'silly', 'verbose', 'warn', 'error'],
    coerce: (level) => {
      log.level = level
      return level
    },
  })

thanks, and maybe it fixes @Alexsey issue too

@tkarls
Copy link

@tkarls tkarls commented Feb 22, 2017

Same problem as original poster.

Think it makes most sense to first restrict the options using choices and then after a valid choice run the coerce function.

That way the two features can coexist. Which is impossible now.

@bspink
Copy link

@bspink bspink commented Feb 14, 2019

Also getting the same problem as the original poster.

IMO it makes sense to run the coerce after choices, but perhaps there could be an option to specify the behaviour (e.g. coerceFirst)? Might be overkill, but just throwing the option out there as it would be nice to at least have the option to do the coercion last.

@bcoe bcoe added the bug label Sep 24, 2020
@bcoe
Copy link
Member

@bcoe bcoe commented Sep 24, 2020

would happily take a patch for this, if someone wants demonstrate the recommended behavior.

bcoe added a commit that referenced this issue Nov 22, 2020
@bcoe bcoe closed this in #1813 Nov 22, 2020
bcoe added a commit that referenced this issue Nov 22, 2020
Fixes #756
@samermurad
Copy link

@samermurad samermurad commented Nov 23, 2020

@bcoe hey there, having the same issue, I have a list of brands, which I specify by name, and the coercion loads theirs options,

so for example: npm run myScript -- --brand=A

is coerced into:

{
  brandName: 'A',
  option1: ....,
  option2: ....,
  ...etc
}

my choices are basically the keys of the available brands, so literally Object.keys(brands).

To be completely honest, my case is a bit more special because I also have a yargs middleware the let's the user select one choice if otherwise not specified using the inquirer npm

So if you run the script without the brand option you'll be prompted to select the desired option.

at first I thought the middleware was the problem but it isn't.

So I actually ended up using only the middleware, something like:

const brandMiddleware = async (argv) => {
  if (!argv.brand) {
    const choiceBrand = await inquireForBrand();
    // coercion code
    return { brand: brands.resolveBrandConfig(choiceBrand) };
  }
  if (typeof argv.brand === 'string') {
     // coercion code
    return { brand: brands.resolveBrandConfig(argv.brand) };
  }
  return {};
};

code above handles both cases correctly, but feels like kind of a messy workaround, as I need to take care of the any aliases manually.

Sorry for the long scroll, just trying to paint the picture.

TL;DR

Under the premise that the .coerce method comes from the yargs-parser, it should be communicated clearly what happens before what on the yargs level, but plainly, from an outside perspective, it makes zero sense to run the coercion code before the choices, the following flow is what I think makes the most sense:
[input] --> [validation] --> [coercion] --> [middleware] --> [command]

@bcoe
Copy link
Member

@bcoe bcoe commented Nov 23, 2020

@samermurad if you can think of a good place to clarify this (perhaps in the middleware section, or under and advanced topic), would appreciate the patch.

@samermurad
Copy link

@samermurad samermurad commented Nov 24, 2020

@bcoe should I just update the documentation accordingly? or should I patch the coerce behavior as well?

@bcoe
Copy link
Member

@bcoe bcoe commented Nov 24, 2020

@samermurad I would rather nudge people towards middleware for performing transforms, and would like to fix the bug that middleware doesn't fire if you have no commands.

The problem with trying to make significant changes to coercion, is that it happens in yargs-parser during the parsing step, not in yargs during validation. I believe that changing this behavior would be a pretty significant overhaul.

@samermurad
Copy link

@samermurad samermurad commented Nov 27, 2020

@bcoe
I totally agree
so I should just add some examples for "sophisticated" middlewares in the advanced section?
This brings up a complicated question regarding the flow and behavior of middlewares vs options/coercion, don't you think? I mean the middlewares technically allow you to do anything, the so it doesn't matter how much you setup your yargs flow, you can end up missing your whole configuration by completely overriding the yargs flow, Why do I need the choices for example if all I need to do is create a middleware and check that the choices are valid? you know? it all seems redundant all of a sudden, one starts to re-think the whole middleware solution in comparison to the rest of the apis, the flows and restrictions should complement each other and not negate each other, otherwise why have the whole pipeline the way it is?

@bcoe
Copy link
Member

@bcoe bcoe commented Nov 28, 2020

@samermurad bother you to open a new issue for this conversation, I'm likely to lose track of this since it's in a thread from 2017.

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

Successfully merging a pull request may close this issue.

8 participants