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

Escape CSON correctly #1109

Merged
merged 8 commits into from Mar 6, 2019

Conversation

Projects
None yet
4 participants
@Aerijo
Copy link
Member

Aerijo commented Feb 14, 2019

Requirements

  • Filling out the template is required. Any pull request that does not include enough information to be reviewed in a timely manner may be closed at the maintainers' discretion.
  • All new code requires tests to ensure against regressions

Description of the Change

Escapes quotes and backslashes better in the keybinding-to-string function.

In addition to linked issue, this is a keybinding I found on my own Atom that needs quote escaping

'atom-text-editor[data-grammar~=\'css\']':
  'ctrl-alt-c': 'css-declaration-sorter:sort'

Without this PR, it wouldn't escape the value quotes.

Also tidied up the composition part, which was really hard to read IMO. Now it's a clean single line with well defined newlines and spacing.

Alternate Designs

Considered CSON.stringify(...), but
(1) it does a lot of extra work for little benefit,
(2) it returns with double quotes, and
(3) it's wrong; trying that with the raw text \\" (escaped backslash + ") ironically gives \", which is an escaped ". (Actually might not be wrong, as \\" would already be escaped by JSON.stringify to \\\". But I don't want to add a new dependency just for one thing)

Could also run JSON.stringify and then swap quote escapes + replace single with double quotes, but I don't think it's necessary.

Benefits

Correctly escape ', ", and \ in strings

Possible Drawbacks

none

Applicable Issues

closes #1110

Aerijo added some commits Feb 14, 2019

@rsese

This comment has been minimized.

Copy link
Member

rsese commented Feb 19, 2019

Almost forget to ask, can you add a test for this? Whoever ends up reviewing will ask 😄

Aerijo added some commits Feb 20, 2019

@Aerijo

This comment has been minimized.

Copy link
Member Author

Aerijo commented Feb 20, 2019

@rsese added specs

@Arcanemagus Arcanemagus added the triaged label Feb 21, 2019

@rafeca

This comment has been minimized.

Copy link
Contributor

rafeca commented Mar 5, 2019

Thanks for the contribution! 😍

Since the escape logic of CSON matches the one in JSON (with the only difference of having to escape single quotes instead of double quotes when using single quote notation), it will be easier to just use JSON.stringify() and change the quoting escaping.

Your escapeCSON function would look like:

const escapeCSON = (input) => {
  return JSON.stringify(input)
    .slice(1, -1) // Remove wrapping double quotes
    .replace(/\\"/g, '"') // Unescape double quotes
    .replace(/'/g, '\\\''); // Escape single quotes
}

This will also help escaping other reserved characters like \n, etc.

What do you think?

@Aerijo

This comment has been minimized.

Copy link
Member Author

Aerijo commented Mar 6, 2019

Yeah, it feels safer to leave the majority of the escaping to the builtin JSON.

@rafeca Done, and the failed spec seems completely unrelated.

@rafeca

rafeca approved these changes Mar 6, 2019

Copy link
Contributor

rafeca left a comment

Awesome!! 🎉

Thanks a lot 😃

@rafeca rafeca merged commit 9f97f0a into atom:master Mar 6, 2019

1 of 2 checks passed

continuous-integration/travis-ci/pr The Travis CI build failed
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details

@rafeca rafeca self-assigned this Mar 12, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.