search

yargs / yargs-parser

:muscle: the mighty option parser used by yargs
http://yargs.js.org/
ISC License
493 stars 118 forks source link

Convert ANSI-C quoted strings to their value #346

Open verhovsky opened 3 years ago

verhovsky commented 3 years ago

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.

gitcoinbot commented 3 years ago

Issue Status: 1. Open 2. Started 3. Submitted 4. Done

This issue now has a funding of 50.0 USDC (50.0 USD @ $1.0/USDC) attached to it.

gitcoinbot commented 3 years ago

Issue Status: 1. Open 2. Started 3. Submitted 4. Done

Work has been started.

These users each claimed they can complete the work by 265 years, 8 months from now. Please review their action plans below:

1) jhboub has applied to start work _(Funders only: approve worker | reject worker)_.

can I try this i need to work on something if possible 2) orderr66 has been approved to start work.

  1. Fork the project and explore the codebase.
  2. Write a unit test for the bug.
  3. Make this test pass.
  4. Write production code.
  5. Create a PR.

Learn more on the Gitcoin Issue Details page.

gitcoinbot commented 3 years ago

@orderr66 Hello from Gitcoin Core - are you still working on this issue? Please submit a WIP PR or comment back within the next 3 days or you will be removed from this ticket and it will be returned to an ‘Open’ status. Please let us know if you have questions!

Funders only: Snooze warnings for 1 day | 3 days | 5 days | 10 days | 100 days

sTEVE7611 commented 3 years ago

gamma(n+1/2)

Re Steven Calcaro

On Tue, Mar 2, 2021 at 9:16 AM Gitcoin.co Bot notifications@github.com wrote:

@orderr66 https://github.com/orderr66 Hello from Gitcoin Core - are you still working on this issue? Please submit a WIP PR or comment back within the next 3 days or you will be removed from this ticket and it will be returned to an ‘Open’ status. Please let us know if you have questions!

  • reminder (3 days)
  • escalation to mods (6 days)

Funders only: Snooze warnings for 1 day https://gitcoin.co/issue/yargs/yargs-parser/346/100024881?snooze=1 | 3 days https://gitcoin.co/issue/yargs/yargs-parser/346/100024881?snooze=3 | 5 days https://gitcoin.co/issue/yargs/yargs-parser/346/100024881?snooze=5 | 10 days https://gitcoin.co/issue/yargs/yargs-parser/346/100024881?snooze=10 | 100 days https://gitcoin.co/issue/yargs/yargs-parser/346/100024881?snooze=100

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/yargs/yargs-parser/issues/346#issuecomment-789068682, or unsubscribe https://github.com/notifications/unsubscribe-auth/ASM4AJRTPD2XBS7UA2IYBCDTBUMNDANCNFSM4VTQWV3A .

kylezervas commented 3 years ago

346

On Tue, Mar 2, 2021 at 11:16 AM Gitcoin.co Bot notifications@github.com wrote:

@orderr66 https://github.com/orderr66 Hello from Gitcoin Core - are you still working on this issue? Please submit a WIP PR or comment back within the next 3 days or you will be removed from this ticket and it will be returned to an ‘Open’ status. Please let us know if you have questions!

  • reminder (3 days)
  • escalation to mods (6 days)

Funders only: Snooze warnings for 1 day https://gitcoin.co/issue/yargs/yargs-parser/346/100024881?snooze=1 | 3 days https://gitcoin.co/issue/yargs/yargs-parser/346/100024881?snooze=3 | 5 days https://gitcoin.co/issue/yargs/yargs-parser/346/100024881?snooze=5 | 10 days https://gitcoin.co/issue/yargs/yargs-parser/346/100024881?snooze=10 | 100 days https://gitcoin.co/issue/yargs/yargs-parser/346/100024881?snooze=100

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/yargs/yargs-parser/issues/346#issuecomment-789068682, or unsubscribe https://github.com/notifications/unsubscribe-auth/ASC6KFZTTFIV57GQWL55VNDTBUMNBANCNFSM4VTQWV3A .

gitcoinbot commented 3 years ago

@orderr66 Hello from Gitcoin Core - are you still working on this issue? Please submit a WIP PR or comment back within the next 3 days or you will be removed from this ticket and it will be returned to an ‘Open’ status. Please let us know if you have questions!

Funders only: Snooze warnings for 1 day | 3 days | 5 days | 10 days | 100 days

gitcoinbot commented 3 years ago

@orderr66 Hello from Gitcoin Core - are you still working on this issue? Please submit a WIP PR or comment back within the next 3 days or you will be removed from this ticket and it will be returned to an ‘Open’ status. Please let us know if you have questions!

Funders only: Snooze warnings for 1 day | 3 days | 5 days | 10 days | 100 days

gitcoinbot commented 3 years ago

@orderr66 Hello from Gitcoin Core - are you still working on this issue? Please submit a WIP PR or comment back within the next 3 days or you will be removed from this ticket and it will be returned to an ‘Open’ status. Please let us know if you have questions!

Funders only: Snooze warnings for 1 day | 3 days | 5 days | 10 days | 100 days

gitcoinbot commented 3 years ago

@orderr66 Hello from Gitcoin Core - are you still working on this issue? Please submit a WIP PR or comment back within the next 3 days or you will be removed from this ticket and it will be returned to an ‘Open’ status. Please let us know if you have questions!

Funders only: Snooze warnings for 1 day | 3 days | 5 days | 10 days | 100 days

bcoe commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

@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 commented 3 years ago

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.