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

Disable Style/PerlBackrefs #40

Merged
merged 1 commit into from Dec 8, 2018
Merged

Conversation

@knu
Copy link
Contributor

@knu knu commented Dec 7, 2018

It is painful to see every occurrence of $n expanded to Regexp.last_match(n) only to get more lengthy and slower code.

For instance, the following piece of code:

str.gsub!(/([a-z]+)_([a-z]+)/) { $1.capitalize + $2.capitalize }

gets corrected to this:

str.gsub!(/([a-z]+)_([a-z]+)/) { Regexp.last_match(1).capitalize + Regexp.last_match(2).capitalize }

and it no longer fits nicely in one line on the screen.

I, as one coming from the Unix background, don't feel like $1-$9 are so cryptic you should avoid them, but is that true for the folks out there?

It is painful to see every occurrence of `$n` expanded to
`Regexp.last_match(n)` only to get more lengthy and slower code.
@rosston
Copy link
Member

@rosston rosston commented Dec 7, 2018

I am not primarily a Rubyist, so what's cryptic for me is probably a little different than others (although I also come from a Unix background).

I would say that, in your example with a block passed to gsub, the global variables don't look cryptic. The block is right near the regex, so it's pretty simple to infer what the $n vars mean (or know what to search the internet for if you want to confirm).

However, in the example you linked to in that Ruby style guide:

/(regexp)/ =~ string
...

# bad
process $1

# good
process Regexp.last_match(1)

I think the global variables are indeed rather cryptic and point to the danger in them being global variables, since they could be used in code very far from the intended regular expression.

@knu
Copy link
Contributor Author

@knu knu commented Dec 7, 2018

@rosston I see your point, but the said example looks rather unfamiliar to me because you should only use $1 if the match actually succeeds. You normally use $1 in a block or in a then/when clause as follows:

case string
when /(regexp)/
  # ...
  process $1
end
# or
if /(regexp)/ =~ string
  # ...
  process $1
end

And I think using $1 in a context introduced with a regexp match makes it less dangerous looking.

Also, I'd like to point out that Regexp.last_match(1) is a call to a global function and no much different from $1 in that they both look like access to global data.

@knu
Copy link
Contributor Author

@knu knu commented Dec 7, 2018

Perhaps the cop should instead suggest using named back references wherever possible.

if /(?<matched>regexp)/ =~ string
  process = matched
end 
@rg-3
Copy link

@rg-3 rg-3 commented Dec 7, 2018

I agree. I see nothing wrong with $0-$9 and by now I would consider them to be well known although way less google-able than Regexp.last_match.

It is painful to see every occurrence of $n expanded to Regexp.last_match(n) only to get more lengthy and slower code.

Right, but seems that rubocop prioritises appearance over everything else, including performance and
functionality.

For example, it has two default cops that mean BasicObect is basically unsupported by this style guide:

1. Style/NilComparison
2. Preferring is_a? (... don't know the cop name)
@rg-3
Copy link

@rg-3 rg-3 commented Dec 7, 2018

I think the global variables are indeed rather cryptic and point to the danger in them being global variables, since they could be used in code very far from the intended regular expression.

Just to be clear, even though $0-9 appear as global variables, they are thread local and cannot be re-assigned:

$1 = 5
SyntaxError: (eval):2: Can't set variable $1
@knu
Copy link
Contributor Author

@knu knu commented Dec 8, 2018

Yes, $1-$9 are method local and thread local, and readonly, but you can still change their values by reassigning $~.

Here's an interesting blog post recently written in Japanese: https://nacl-ltd.github.io/2018/11/08/gsub-wrapper.html (Google Translate)

@searls
Copy link
Member

@searls searls commented Dec 8, 2018

How about we just disable this? It doesn't sound like either style is consistently preferable and it's not a very important point

@rg-3
Copy link

@rg-3 rg-3 commented Dec 8, 2018

Cheers it's always good to learn something new :)

@searls searls merged commit fc2f5f9 into testdouble:master Dec 8, 2018
@knu knu deleted the knu:disable-PerlBackrefs branch Dec 9, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants