Ideas for some new %-style literal cops

[bbatsov]

#834 reminded me about some cops, that might be nice to have:

• A cop that checks for redundant use of %q or %(Q). Obviously it's always a better idea to use '' and "" unless you really need %q or %Q. And sometimes you might have used %Q when %q would do.
• A cop that checks for the use of %x where ```` would do.
• A cop that checks for the use of %W where %w would do.

All of those should be fairly easy to implement. Feel free to hack on any of them. //cc @yujinakayama @jonas054 @hannestyden

[bbatsov]

@jonas054 Would like this task? I think that writing a few cops might be a welcome break for you from fixing bugs. :-)

[jonas054]

Yes, that might be nice. I'm putting it on my todo list.

[sfeldon]

Newbie here. I've got a first stab at %W vs. %w that I'm polishing up. My main question right now is that I borrowed some helper methods from percent_literal_delimiters.rb and don't know where I should factor them out to. Anyone got a few minutes to help me acclimate to the conventions of the project? :)

[jonas054]

@sfeldon Thanks for helping out!

Create a new module under lib/rubocop/cop/mixin and include it from the two cops.

[bbatsov]

@sfeldon I'd suggest extracting the common code into a mixin or a force. You can see examples of this done in IfUnlessModifier (the mixin approach) and VariableForce (the force approach).

[sfeldon]

Examining the force code, I've decided that I could have it done with a mixin before I could understand how to do it with a force. Thanks for the info, gents--working on it.

[jonas054]

@sfeldon Any progress?

[bbatsov]

His cop is already merged.

On Sunday, April 27, 2014, Jonas Arvidsson [email protected] wrote:

@sfeldon https://github.com/sfeldon Any progress?

—
Reply to this email directly or view it on GitHubhttps://github.com//issues/835#issuecomment-41488662
.

Best Regards,
Bozhidar Batsov

http://www.batsov.com

[jonas054]

I forgot about that. I'll get started on the rest then.

[sfeldon]

Aigh. If you were holding on working on stuff because of me, please
don't--I'm still targeting trying to add autocorrect to the cop I added for
the experience, but I went from being paid to be idle to running two
projects when my boss had kidney failure. Please don't wait for me right
now.

And thank you both for the support and the experience. :)

steve

On Sat, Apr 26, 2014 at 11:36 PM, Jonas Arvidsson
[email protected]:

I forgot about that. I'll get started on the rest then.

—
Reply to this email directly or view it on GitHubhttps://github.com//issues/835#issuecomment-41489577
.

[jonas054]

@sfeldon No worries! I'll leave the autocorrect for the %W cop till last in case you find the time to do it, and I'll start working on the %x cop soon.

[jonas054]

%q / %Q is a bit trickier than other % literals. I have a couple more cops planned after UnneededPercentQ. One that checks if %Q is used when %q would do. Should be similar to StringLiterals with the same kind of configuration option. The other checks for %() vs. %Q(). Should also be configurable.

[bbatsov]

Sounds good. —
Sent from Mailbox

On Sat, Jun 21, 2014 at 10:33 AM, Jonas Arvidsson
[email protected] wrote:

%q / %Q is a bit trickier than other % literals. I have a couple more cops planned after UnneededPercentQ. One that checks if %Q is used when %q would do. Should be similar to StringLiterals with the same kind of configuration option. The other checks for %() vs. %Q(). Should also be configurable.

Reply to this email directly or view it on GitHub:
#835 (comment)

[dkubb]

My rules for when to use ' / ", and %q / %Q are a bit different and I figured I'd mention them in case it hasn't be discussed before. If there is a better forum for this discussion please let me know.

For literal strings, my primary concern is to distinguish strings that use variable interpolation vs those that don't. When I see a string quoted with ' or %q I know that it won't be modified at runtime, and that I don't need to inspect it further to see what extra logic could be embedded within it. If I see a string quoted with " or %Q then I know I need to dig further in order to understand what it does.

I always try to quote a string using ' if I can, and fallback to using %q only when the string includes a single quote because I think backslash escaped strings are more difficult to read. I'll only ever use " when I want variable interpolation, and %Q when I want variable interpolation in a string that already uses double quotes.

However the UnneededPercentQ cop recommends I use " when the string contains a single quote. I consider reducing the scope of allowed states (albeit slightly in this case) in my program to be more important than this rule and will likely disable it. I feel like it conflicts with the "Option A" rule outlined in the style guide for string literals -- arguably the more preferred style by the authors of the guide since they decided to adopt that convention throughout.

[jonas054]

@dkubb I think your style preference makes sense, and hopefully we can support it as a configuration choice. The current implementation of StringLiterals allows "'" even when EnforcedStyle is single_quotes. The UnneededPercentQ cop follows the same path and prefers "'" over %q('). This feels right as a default to me, as it favors symbols over letters. It's somewhat similar to preferring a + b over a.add(b) IMO, and I generally choose a symbol rather than text when possible.

The style guide doesn't go into much detail, and really doesn't cover these corner cases, so I wouldn't say we're in violation of it as it is right now.

[jonas054]

I guess that's all the cops you originally listed, @bbatsov. Wow, that took a while. It's July already. 😄

We can keep the issue open a little longer while I try to come up with a solution to support the style described by @dkubb.

[jonas054]

@dkubb Your preferred style could not be supported without adding too much complexity. That brings this issue to a close.