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

fix: enable ansi-c escape quoting #365

Closed
wants to merge 2 commits into from
Closed

Conversation

jonluca
Copy link

@jonluca jonluca commented Mar 11, 2021

Fix for #346

I think there should be at least a little discussion around this change.

In general, this seems very safe. it's effectively only fixing bugs. It doesn't go all the way, though - in bash, the escape sequence with \b looks like this:

$ echo $'fixx\b me'
fix me

where as in this version it'll print out fixx me. I'm not sure implementing character deletion makes sense, though - perhaps we could hide actual mutations to the input like this through an option?

In general this code path is going to be hit so rarely that I don't think it matters that much. Just think people should be aware of it.

There's also escape sequences like bell which we silently strip out.

@verhovsky
Copy link

verhovsky commented Mar 11, 2021

echo $'fixx\b me' prints "fix me" because echo prints the "x" then backspaces over it, but the underlying data still has the escape code:

$ echo $'fixx\b\b\b\b\b\b me' | xxd
00000000: 6669 7878 0808 0808 0808 206d 650a       fixx...... me.
$ node -e "console.log(process.argv);" $'fixx\b\b\b\b\b\b me'
[ '/usr/bin/node', 'fixx\b\b\b\b\b\b me' ]

So I think it should keep the escape codes.

@jonluca
Copy link
Author

jonluca commented Mar 12, 2021

@verhovsky You're right, I agree. Updated the diff.

fix: lint

fix: more code coverage

fix: coverage

fix: ts

fix: global search and replace

feat: return all escaped characters

fix: types

fix: typescript

fix: lint

fix: updated comment
@jonluca
Copy link
Author

jonluca commented Mar 19, 2021

@bcoe what's the process for getting the PR landed? Is there anything else you need from me?

@jonluca
Copy link
Author

jonluca commented Mar 19, 2021

Actually @verhovsky https://github.com/yargs/yargs-parser/pull/366/commits seems more thorough and with better tests, let's land that one instead

@bcoe bcoe closed this Mar 31, 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.

3 participants