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

Convert ANSI-C quoted strings to their value #346

Open
verhovsky opened this issue Dec 24, 2020 · 13 comments
Open

Convert ANSI-C quoted strings to their value #346

verhovsky opened this issue Dec 24, 2020 · 13 comments
Labels

Comments

@verhovsky
Copy link

verhovsky commented Dec 24, 2020

This makes sense:

> yargs.parse('echo --data "\\n"');
{ _: [ 'echo' ], data: '\\n', '$0': '' }
> yargs.parse("echo --data '\\n'");
{ _: [ 'echo' ], data: '\\n', '$0': '' }

This could be better:

> yargs.parse("echo --data $'\\n'");
{ _: [ 'echo' ], data: "$'\\n'", '$0': '' }

In bash, strings of the form $'string' are a special type of string:

$ echo $'some\\ntext'
some\ntext
$ echo 'some\\ntext'
some\\ntext

See https://www.gnu.org/software/bash/manual/bash.html#ANSI_002dC-Quoting

It would be helpful if yargs returned the raw value for these types of strings as well, instead of making the user parse it themselves.

@bcoe bcoe transferred this issue from yargs/yargs Jan 4, 2021
@bcoe bcoe added the feature label Jan 4, 2021
@gitcoinbot

This comment was marked as spam.

@gitcoinbot

This comment was marked as spam.

@gitcoinbot

This comment was marked as spam.

@sTEVE7611

This comment was marked as spam.

@kylezervas
Copy link

kylezervas commented Mar 4, 2021 via email

@gitcoinbot

This comment was marked as spam.

1 similar comment
@gitcoinbot

This comment was marked as spam.

@gitcoinbot

This comment was marked as spam.

@gitcoinbot

This comment was marked as spam.

@bcoe
Copy link
Member

bcoe commented Mar 15, 2021

I don't know this problem space super well, but it seems that @aduh95's concern on Node.js is that this syntax is only in the bash shell?

I'm concerned mainly because this would be a breaking change -- and I wonder if it's something that would be better handled in user-land -- I'm on the fence mind you, potentially open to the feature.

@bcoe
Copy link
Member

bcoe commented Mar 15, 2021

One thought, why don't we just add a config option for the feature, --bash-ansi-strings or something like that? then you can enable it upstream in your library.

@verhovsky
Copy link
Author

verhovsky commented Mar 15, 2021

@bcoe please ignore the Node issue, I didn't understand how exec works. The reason the issue was closed in Node is because it has nothing to do with Node's exec. It already works because exec just passes the string you give it to a shell. The default shell it uses is sh, which doesn't have this syntax so sh gets that weird quote syntax string and treats it as a $ followed by a quoted string (or whatever), not a special quoted string. If you tell exec to pass the given command string to bash instead, with

exec(
  "printf '%s' $'hello'",
  { shell: '/bin/bash' },
)

then bash parses the $'' string as its special string format correctly.

I don't know this problem space super well

In bash, if you try to print "\n" it won't print a real newline character, it'll print 2 characters: a backslash followed by an "n" (followed by an actual newline because echo always adds a newline at the end)

$ echo '\n'
\n
$

You can use ANSI-C quoting to convert backslashed characters to the actual character that you can't really input in the terminal

$ echo $'\n'


$

this would be a breaking change

Assuming the parsing is correct, then not exactly but it's not completely transparently backwards compatible either. If someone has already written code that parses these quotes, then now that they are parsed by yargs, and since regular " and ' quotes are already stripped by yargs, all they would notice is that suddenly they never see those $'' quotes anymore, but always just regular parsed out strings with no quotes instead. Of course, if they are using sh in parallel (as in they're parsing args with yargs and also running the original string with sh or something) and now yargs is parsing arguments differently from their shell, that could cause issues, but it's a problem right now from the other side, people using bash are having their arguments parsed differently than they are by yargs.

add a config option for the feature

Where does that go, is this something you like pass to npm when you install it? I was thinking of doing something like

parse("echo -n $'my \\n string'", { shell: 'bash' })

and have shell: 'sh' as the default, then you don't get that parsing unless you ask for it.

Also, thanks for muting @gitcoinbot.

@verhovsky
Copy link
Author

I added some skipped tests to my PR that exhaustively try to compare all possible literals like \U10FFFF against what the result of printing them through bash with exec would be.

I also see now that there's no real good place in the signature for parse to put a shell argument, so forget what I said about that, I don't know.

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

Successfully merging a pull request may close this issue.

5 participants