search

atom / toggle-quotes

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

Allow quote toggling in unrecognized (non-string as reported by the grammar) buffer areas #45

Open jkeen opened 7 years ago

jkeen commented 7 years ago

Description Of The Change

This fix provides a fallback if the current grammar doesn't report the current buffer as .string.quoted. This happens in cases where the quote characters have been customized, most commonly when the user has added a es6-style backtick as a quote character.

When toggling quotes in a grammar that reports these regions correctly, you can toggle from 

"hello" -> 'hello' -> `hello`

but after that when it should cycle back to the first quote character, it stops working because the current buffer area is no longer recognized as .string.quoted. A fallback was already in place to check for a .invalid.illegal region, but that doesn't always work. Now, if no region has been found, it looks in the current scope.

Alternate Designs

I kept the check for the editor.bufferRangeForScopeAtPosition('.invalid.illegal', position) before my added code, but I'm not positive that's necessary anymore. It doesn't seem to be hurting anything.

Why Should This Be In Core?

Because it looks like this package hasn't received any love in a long time, and judging by the outstanding issues and PRs this might resolve some problems.

Benefits

Better quote toggling for the masses. Toggling in more file types.

Possible Drawbacks

Sometimes people hate change, even if it's good.

Applicable Issues

This resolves a number of reported issues: #35, #31, and #43, #42, and #9

50Wliu commented 7 years ago

Off-topic: @lee-dohm it looks like this repo is using an incorrect PR template reserved for atom/atom.

lee-dohm commented 7 years ago

@50Wliu It appears they copied the template from atom/atom ... there isn't a PR template in this repository :grinning:

jkeen commented 7 years ago

@lee-dohm @50Wliu Sorry, I incorrectly assumed that the other pull requesters just didn't read the directions. My bad. 👓 😜

At the bottom of the toggle quotes readme it said under the Contributing header that "Whether it's filing bugs and feature requests or working on some of the open issues, Atom's contributing guide will help get you started while the guide for contributing to packages has some extra information.", so I followed that link and read the Pull Requests section… which is why I filled it out in this verbose format.

Now I know. 💫

50Wliu commented 7 years ago

Is there a reason we can't simply look for .string instead, or both .string.quoted and .string.interpolated?

lee-dohm commented 7 years ago

I'd also rather see the quote rotation be an attribute on the grammar like commentStart and the indent increase/decrease patterns.

50Wliu commented 7 years ago

Yes. That would be cool.

jkeen commented 7 years ago

Besides it being convenient, is there another reason why we're relying on the grammar to tell us where the quotes (that we specify in the toggle-quotes config) are?

Couldn't we just remove that dependence and search for the specified quote characters on the current line? If we did that, wouldn't that remove the dependence of having to have each file's grammar correctly report a custom quote like a backtick as a quoted region?

@50Wliu's comment the other night got me thinking about that that direction, and I started a refactor that does that. It's not done yet (I added a couple of failing test cases) but for the primary use cases it's working in any file type.

Just wondering about the downsides I'm missing to an approach like that, being new to atom hacking and not too familiar with the ins and outs of grammars, etc.

lee-dohm commented 7 years ago

Besides it being convenient, is there another reason why we're relying on the grammar to tell us where the quotes (that we specify in the toggle-quotes config) are?

Unless I'm misunderstanding your design, this won't support people who work in multiple languages very well. Except for the case where they work in languages that all use the same set of quotes.

50Wliu commented 7 years ago

Powershell, for example, uses backticks as an escape character, and Clojure uses single quotes as an escape character.

jkeen commented 7 years ago

Ah, didn't realize that about Powershell and Clojure.

I'm mainly in ruby and javascript, and the problem I was encountering was that in ES6 a backtick is a valid quote, so I added it to quotes list in the toggle quotes config. But then when toggling quotes in ruby, it would cycle from " -> ' -> `, and once it replaced the quoted region with backticks, the grammar wouldn't recognize the region as string.quoted anymore so toggle quotes wouldn't toggle back.

That's mainly what I was trying to resolve by not relying on the grammar to say "here's the quoted region; look in here". Now it just looks at the entire line for content.

If the grammar would report the quoted region and the valid quote list, that'd be something. Maybe that's the path to fix some of these issues?

EDIT: Oh, that's what you suggested @lee-dohm, isn't it. Yeah, that. :)