Reconsider handling of empty string values

[rgov]

bad-input: empty string added to unknown values

I'm not sure this is a valuable test. It's not clear to me that this is checking for desirable behavior. However, it is expected that you should be able to pass a blank value, such as sed -i ''.

In this specific case, where we have a option=value argument that accepts multiple values and one of the values is blank, it seems fine to me to have this be undefined behavior (i.e., no test) or define it to work the way that other blank values should work.

(Note I think this handling of --foo=bar baz is probably non-standard and if so it should be removed. Are there any examples of projects that use this?)

runner.test('bad-input: empty string added to unknown values', function () {
  const optionDefinitions = [
    { name: 'one', type: String },
    { name: 'two', type: Number },
    { name: 'three', type: Number, multiple: true },
    { name: 'four', type: String },
    { name: 'five', type: Boolean }
  ]
  const argv = [ '--one', '', '', '--two', '0', '--three=', '', '--four=', '--five=' ]
  a.throws(() => {
    commandLineArgs(optionDefinitions, { argv })
  })
  a.deepStrictEqual(commandLineArgs(optionDefinitions, { argv, partial: true }), {
    one: '',
    two: 0,
    three: [ 0, 0 ],
    four: '',
    five: true,
    _unknown: [ '', '--five=' ]
  })
})

[75lb]

Yes, it's not the most realistic test. It exists to make a record of what happens when users try to pass an empty string via --option= or --option '' alongside other option definitions such as a multiple.

Even if we break this test we'd need a new major version.

[rgov]

import argparse
parser = argparse.ArgumentParser()
parser.add_argument('values', nargs='*')
args = parser.parse_args(['a', '', 'b'])
print(args)

Returns Namespace(values=['a', '', 'b']) which seems to suggest that argparse doesn't consider blank values separately. I would follow its lead. Whitespace is handled by the lexer; if something blank reaches the parser, then it's probably intended to be there.

[75lb]

Sometimes process.argv contains empty strings, whether the user intended to pass them in or not. We just need to ensure empty strings are considered valid option values and that it's possible to set them in the usual ways.

• --one ''
• --one=''
• -o ''
• etc

[rgov]

Agreeed. Is this testing properly?

[75lb]

Is this testing properly?

Partially. I think the tests regarding empty-string option values should be improved and make it more clear what the defined behaviour is. I'll look into this when I get time.

I think this handling of --foo=bar baz is probably non-standard

I don't understand what you are highlighting, here. I think this is an acceptable pattern:

$ something --view=detail file.js

[rgov]

Does that set view=['detail', 'file.js']? If so, I would like to know if there are existing command line tools that support this syntax. I cannot think of an example.

[75lb]

No, it would return options looking something like this: (assuming files is a multiple defaultOption)

{
  view: 'detail',
  files: [ 'files.js' ]
}

[75lb]

I think this handling of --foo=bar baz is probably non-standard and if so it should be removed

Maybe you misunderstood something because we don't try to handle a syntax case where --foo=bar baz sets two values (bar and baz) on one option (--foo).

[rgov]

Isn't that what this test is doing? It takes the argv values [ '--three=', '' ] and sets three: [ 0, 0 ].

[75lb]

both args in this array [ '--three=', '' ] set empty strings.. Number('') evaluates to 0, so the end result is three: [ 0, 0 ].

[rgov]

we don't try to handle a syntax case where --foo=bar baz sets two values (bar and baz) on one option (--foo).

both args in this array [ '--three=', '' ] set empty strings.

Which is it?

[75lb]

I have removed the confusing test cited in your original post and added more clear tests for handling empty strings (below). Once ready, I'll push these tests (and other changes) to your fork so we can finish off the PR.

runner.test('bad-input: empty string', function () {
  const optionDefinitions = [
    { name: 'one' }
  ]
  const argv = [ '--one', '' ]
  a.deepStrictEqual(commandLineArgs(optionDefinitions, { argv }), {
    one: ''
  })
})

runner.test('bad-input: empty string, option cluster', function () {
  const optionDefinitions = [
    { name: 'one', alias: 'o', type: Boolean },
    { name: 'two', alias: 't' }
  ]
  const argv = [ '-ot', '' ]
  a.deepStrictEqual(commandLineArgs(optionDefinitions, { argv }), {
    one: true,
    two: ''
  })
})

runner.test('bad-input: empty string, --option=', function () {
  const optionDefinitions = [
    { name: 'one' }
  ]
  const argv = [ '--one=' ]
  a.deepStrictEqual(commandLineArgs(optionDefinitions, { argv }), {
    one: ''
  })
})

runner.test('bad-input: empty string unknown value', function () {
  const optionDefinitions = [
    { name: 'one', type: Boolean }
  ]
  const argv = [ '--one', '' ]
  a.throws(
    () => commandLineArgs(optionDefinitions, { argv }),
    /UNKNOWN_VALUE/
  )
})

runner.test('bad-input: empty string added to unknown values', function () {
  const optionDefinitions = [
    { name: 'one', type: Boolean }
  ]
  const argv = [ '--one', '' ]
  a.deepStrictEqual(commandLineArgs(optionDefinitions, { argv, partial: true }), {
    one: true,
    _unknown: [ '' ]
  })
})

[Lonli-Lokli]

Was it merged? I am observing (on Windows) treating empty string '' as string with two quotes ''

[75lb]

@Lonli-Lokli Could you post what you are observing in a new issue please, preferably with a reproduction case (some code I can run to see and debug the issue).. thanks.

[Lonli-Lokli]

@75lb I am trying but with no luck, unfortunately - cannot make tests running on my Win10.

Do you have Win somewhere? My Assumption that this test fail test\type-string.mjs

 a.deepStrictEqual(
    commandLineArgs(optionDefinitions, { argv: ['--one', ''] }),
    { one: '' }
  )

[Lonli-Lokli]

@75lb Im still unable to make tests run under Windows10 (esm-runner cannot find files) , but I see in the project that empty string is treated as string with two single quotes.

Can you try to reproduce it?

[75lb]

Hi @Lonli-Lokli - this project is next on my list to look at, will get back to you soon..