Better escaped character support

Aerijo commented 5 years ago

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

This replaces the part that goes through and escapes / unescapes existing quote characters.

Previously, it looked for \' and removed the \, and saw " and escaped it to \". This doesn't work when characters are escaped though, because it would incorrectly change \\'

Now it does a single pass using a regex that grabs both cases. Based on the match it made, it will 1) Escape if it's a lone quote we are about to change the delims to 2) Unescape it if it's an escaped old quote character.

I think this behaviour should be configurable, but is not in the scope of this PR. 3) Ignore it if it's an escaped anything else

As the replace function resumes at the end of a previous match, the "ignore it if it's an escaped anything else" rule lets us count escapes correctly.

Alternate Designs

Iterating the string may have been better, but I still found regex easier to work with.

Benefits

No longer does weird behaviour when escapes are in a string.

Possible Drawbacks

It still assumes the escape character is \. Again, that's existing behaviour and needs it's own PR.

Applicable Issues

Fixes https://github.com/atom/toggle-quotes/issues/33

Aerijo commented 5 years ago

I think the specs should be clearer now. Should it have a warning in that block that backslashes need to be halved when testing manually?

jasonrudolph commented 5 years ago

I think the specs should be clearer now.

@Aerijo: Thanks for these updates, and thanks for taking the time to explain things. :bow:💟

jasonrudolph commented 5 years ago

toggle-quotes v1.1.4 is now available with these updates.

atom / toggle-quotes