atom / toggle-quotes

An Atom package to toggle between single and double quotes
MIT License
77 stars 36 forks source link

Ensure toggling works in tree-sitter grammars with unorthodox strings. #65

Closed savetheclocktower closed 5 years ago

savetheclocktower commented 5 years ago

Description of the Change

Problem

I remarked here about a change that seemed like it introduced a regression: in order for the package to determine if it was inside a toggle-able string when the current file is using a tree-sitter grammar, it queries for syntax nodes with a function predicate that is too brittle. Any string that doesn’t begin and end with a single quote or double quote is guaranteed to get ignored.

But sometimes it shouldn’t be ignored. Two examples:

Solution

The predicate needs to take the (possibly scope-specific) quote character setting into account. So we need to build a predicate at command invocation time using the configured quote characters.

Hence we build a regex on the fly that we use to test whether the syntax node’s text begins and ends with the same quote character. The pattern also optionally allows for one of Python’s string prefixes.

The breaking specs are no longer breaking. I also added a test for the backtick scenario in JavaScript.

Alternate Designs

I can’t think of any. I don’t know enough about tree-sitter grammars to go too deep on this. Happy to hear other solutions.

Benefits

This package will work identically whether tree-sitter grammars are enabled or disabled.

Possible Drawbacks

Performance, perhaps? We build the regex only once per invocation of the toggle-quotes:toggle command, but perhaps it’s more costly to call RegExp#test inside of the predicate than to call String#startsWith and String#endsWith. Can’t imagine the difference is meaningful, though.

Applicable Issues

The regression was introduced in #58. Was going to open an issue for it, but ended up writing this PR instead.

maxbrunsfeld commented 5 years ago

Thanks for the great explanation. This fix looks good to me.

maxbrunsfeld commented 5 years ago

@savetheclocktower Your fix has been published in toggle-quotes version 1.1.3.