[Fix #1460] Allow alias in lexical class/module scope

[marxarelli]

Fixed Style/Alias cop to allow the use of alias in lexical class and
module scope, as the style guide does not forbid it.

[bbatsov]

The suggested changes don't make much sense - after all you can't actually use alias_method outside of a class/module (something we don't actually check currently).

[marxarelli]

I'm not sure I understand your point. You certainly can use alias_method outside of lexical class/module scope.

Object.class_exec { alias_method :id, :object_id }
# or
obj = Object.new
obj.singleton_class.class_exec { alias_method :id, :object_id }

Maybe I'm misunderstanding how the AST parser works (very possible; I just sort of jumped into this) but I'm attempting to omit the enforcement of alias_method over alias in simple use cases like these ...

# OK
class Person
  def first_name
    names.first
  end

  alias given_name first_name
end

... but continue to disallow alias in most other cases, such as ...

obj.singleton_class.class_exec { alias id object_id }

Personally I wouldn't much care about the latter usage either but, then again, I'm generally confused about where we're asserting alias as bad. The style guide doesn't provide any reasoning and, without that, it's difficult to achieve consistency between the rule and the cop. (See rubocop/ruby-style-guide#377)

[bbatsov]

My point is that you can use alias for something like:

alias println puts

[marxarelli]

That's true and possibly indicates other problems with this cop, but how does it relate to my PR? I've made no changes to the way your example might be checked—evidenced by the existing specs that still pass.

[bbatsov]

The problem is that the cop specs are actually flawed and your changes are actually affecting its current behaviour. The cop was intended specifically to disallow alias in a lexical scope as this is the problem that most people have with alias.

[marxarelli]

The comment left in rubocop/ruby-style-guide#377 by @meagar, and confirmed by you, states that the guide "doesn't forbid you from using alias when it's the more appropriate option," which implies that the use case I've put forth in that issue and here is indeed "appropriate" usage. You also linked to an article for further reading on this alternative and implicitly valid perspective.

I'm not trying to be confrontational or anything, I'd just like to bring this cop (and others) more in line with the rules. To me, the best thing about the style guide you've started (and why we're trying to adopt it at Wikimedia) is that it's community driven. However, when cops are introduced that either having no correlation to the guide or are incongruent with the discussion/consensus therein, it seems to undermine the participatory nature of it all.

[bbatsov]

I'm not saying that I'm against the proposed change; I'm saying you should consider its implications for the existing users, which will likely be surprised that the behaviour of the cop was changed without any way for them to use the old behaviour. I'm also saying that the current implementation (before your patch) is flawed, therefore we can't rely on the tests. Perhaps it'd actually be best to make this cop look for just alias in the body of a class/module and disable it by default (which as I said earlier was its original purpose).

We should also update the style guide, of course, as the current state of the rule is less than ideal.

However, when cops are introduced that either having no correlation to the guide or are incongruent with the discussion/consensus therein, it seems to undermine the participatory nature of it all.

Yeah, guess it might feel this way sometimes for which I truly apologise. Unfortunately I don't have that much time on my hands to review everything carefully and from time to time cops that are out of sync with the guide get introduced/enabled. I'm always willing to work towards fixing this.

[bbatsov]

P.S. We can also make this cop configurable, of course. By default it will enforce the new style, but there'll be an option to enforce the current style as well.

[marxarelli]

I'm not saying that I'm against the proposed change; I'm saying you should consider its implications for the existing users, which will likely be surprised that the behaviour of the cop was changed without any way for them to use the old behaviour.

I see where you're coming from. It appears we're both trying to achieve "least surprise"—me from the standpoint of a new RuboCop user, and you from the standpoint of a current one.

Unfortunately I don't have that much time on my hands to review everything carefully and from time to time cops that are out of sync with the guide get introduced/enabled. I'm always willing to work towards fixing this.

Hey, no worries; that's completely understandable. I wonder if a policy on discussion before implementation would be helpful in mitigating divergence. In other words, defer requests for new cops or PRs that change cop behavior to the style guide repo for discussion first—at least before anything gets merged. The style guide is in a pretty good state overall (easily the best Ruby style guide out there), and it might benefit from "slowing its roll" on new rules anyhow. :)

We should also update the style guide, of course, as the current state of the rule is less than ideal.

Cool! Let's continue the discussion in rubocop/ruby-style-guide#383, get the style guide in order, then I'll come back to this and refactor accordingly.

I'm also saying that the current implementation (before your patch) is flawed, therefore we can't rely on the tests.

I totally didn't get that. Sorry, my brain is fried today.

[bbatsov]

OK, now we can move forward with this. As I said earlier the cop has to have two supported styles - the current and the new. They can be named something like PreferAlias and PreferAliasMethod.

[bbatsov]

@marxarelli Now that the guide is updated you can continue with the changes here.

[bbatsov]

@marxarelli ping :-)

[bbatsov]

Closing due to lack of activity.