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

Coerce function returns option of type "number" as "string" #182

Merged
merged 1 commit into from
Jun 22, 2019

Conversation

juergba
Copy link
Contributor

@juergba juergba commented Jun 8, 2019

Description

closes #176

Description of Change

  • applyCoercions(): the return value of the coerce function is now checked by calling maybeCoerceNumber(), wether it should be converted to type number.
  • maybeCoerceNumber(): currently return values of coerce functions are not converted to type number.
    I deleted this restriction <== THIS HAS TO BE CONFIRMED
  • extend one existing test

@juergba juergba changed the title coerce function with option type number Coerce function returns option of type "number" as "string" Jun 8, 2019
Copy link
Member

@bcoe bcoe left a comment

Choose a reason for hiding this comment

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

my concern with this patch is folks who are trying to return the string "55" from coerce, and instead end up with the value 55 because we automatically parse numbers.

What if, instead, we apply the type casting to a variable before passing it to coerce, and always use the value returned by coerce -- I think this would be less shocking.

@juergba
Copy link
Contributor Author

juergba commented Jun 9, 2019

[...] because we automatically parse numbers.

It depends on the option type wether we parse or not. The current behavior (without coerce functions) is:

  • option is number: we parse numbers, configuration parse-numbers doesn't matter
  • option is string: we don't parse numbers, configuration parse-numbers doesn't matter
  • option is undefined: behavior depends on configuration parse-numbers

So if users define their option as string, they will get string "55".
It's clearly a bug when the coerce function returns a string into an option of type number.

The key question finally is whether the option type has precedence over the return type of the coerce function.

@bcoe
Copy link
Member

bcoe commented Jun 9, 2019

The key question finally is whether the option type has precedence over the return type of the coerce function.

Can you explain the bug to me that this solves for you in mocha, was coerce returning a number but it gets turned into a string?

@juergba
Copy link
Contributor Author

juergba commented Jun 9, 2019

const parse = require('yargs-parser');

const args = parse('--retries 2 --retries 3', {
    number: ['retries'],
    coerce: {
        'retries': v => Array.isArray(v) ? v.pop() : v
        // 'retries': v => Number(Array.isArray(v) ? v.pop() : v)
    },
    configuration: {
        'parse-numbers': true,               // default
        'duplicate-arguments-array': true    // default
    }
}
);

console.log(args);   // { _: [], retries: '3' }

In short: a number[] is passed to coerce which returns a string to a number option.
It also returns a string when just a number is passed to coerce.
The alternative with Number(...) correctly returns a number.

This is not a critical bug for Mocha, so don't wait with cutting your new release.

@bcoe bcoe merged commit 2f26436 into yargs:master Jun 22, 2019
@juergba juergba deleted the issue/176 branch June 23, 2019 07:12
This was referenced Mar 14, 2021
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.

Coerce function returns option of type "number" as "string"
2 participants