Add StructInheritance cop

[mmozuras]

No description provided.

[bquorning]

As travis says, when running Rubocop against this file:

lib/rubocop/cop/style/struct_inheritance.rb:20:11: C: Use a guard clause instead of wrapping the code inside a conditional expression.
          if struct_construtor?(superclass)
          ^^

So something like:

def on_class(node)
  _name, superclass, _body = *node
  return unless struct_construtor?(superclass)

  add_offense(node, superclass.loc.expression, MSG)
end

[mmozuras]

@bquorning done. Sorry, completely forgot to check back for Travis status.

[bquorning]

Possible typo: struct_construtor? → struct_constructor?

[mmozuras]

Fixed 😱

How did that happen... 😆

[bbatsov]

There should be an example using Struct.new ... do ... end as well.

[mmozuras]

@bbatsov there already is one or do you mean something else?: https://github.com/bbatsov/rubocop/pull/1571/files?diff=unified#diff-2acbcfc063c998c3c95c51657c537eebR26

[bbatsov]

You can subclass the result of Struct.new do end as well.

[mmozuras]

@bbatsov ohhh... Got it, done 😄

[bbatsov]

Overall the code looks good (apart from my remarks). Next step - checking for classes that can be replaced by a struct. :-)

[mmozuras]

@bbatsov could you give an example of what you mean? 😄

[bbatsov]

I meant that once we're ready with this cop we could create another one checking for code like:

class Person
  attr_accessor :age, :name

  def initialize(age, name)
    ...
  end
end

We could search for more complex patterns, but I think we shouldn't overdo this. Anyways, this was just a random comment. Let's wrap this PR first.

[mmozuras]

@bbatsov changed the message to match the updated one in ruby-style-guide.

[bbatsov]

Shouldn't the example code be indented with two more spaces?

[mmozuras]

@bbatsov 👍

[bbatsov]

One final remark. Keep just the first sentence of the MSG (long messages look bad with some formatters) and wrap Struct.new in backquotes.

[mmozuras]

@bbatsov done 😄

[bbatsov]

👍 Thanks!

[tamird]

There's a small gotcha with Struct.new, which I think this cop doesn't handle:

Person = Struct.new(:first, :last) do
  SEPARATOR = ' '.freeze
  def name
    [first, last].join(SEPARATOR)
  end
end

is not equivalent to:

class Person < Struct.new(:first, :last)
  SEPARATOR = ' '.freeze
  def name
    [first, last].join(SEPARATOR)
  end
end

The former creates ::Person and ::SEPARATOR, while the latter creates ::Person and ::Person::SEPARATOR.

[mocoso]

Why is it better not to inherit from Struct.new?

[Koronen]

@mocoso:

Don't extend an instance initialized by Struct.new. Extending it introduces a superfluous class level and may also introduce weird errors if the file is required multiple times.

https://github.com/bbatsov/ruby-style-guide#no-extend-struct-new

[mocoso]

@Koronen Thanks for the clarification (and for your very polite encouragement for me to read the docs) 😄

[nanodeath]

Just upgraded and got bit by this cop. Can the rule be updated to only address empty class blocks (e.g. no additional methods or constants)? As @tamird alluded to, there are valid use cases for extending Struct.new. Or at the very least, add more details about why this cop should be included and on by default.

[bbatsov]

There's a link to the style guide already.

[nanodeath]

@bbatsov Yes, I saw that, and it didn't clarify anything.

[bbatsov]

Guess you've never seen Struct.new do ...? Check out the official docs http://ruby-doc.org/core-2.2.0/Struct.html

[nanodeath]

I've seen that too -- it was in one of the examples earlier in this thread. But as was already pointed out, it's not equivalent.

I'm just looking for a reason for the hate for this obviously useful pattern.

[bbatsov]

I understand this well enough. Perhaps we should account for the constant definition, but outside of this corner-case the two setups are pretty much the same and it's generally better to prefer the simpler option (simpler inheritance chain).

[bquorning]

I just found this interesting article about defining constants inside a Struct: http://blog.55minutes.com/2012/01/trivia-constants-in-ruby-192-vs-193/

I think I agree with its conclusion: maybe you should just use a simple Class instead.

[bbatsov]

Sounds good to me. @bquorning Please, log a ticket so we don't forget to implement a check for constants defined in structs eventually.

[nanodeath]

I was unaware of the perils of constants defined inside blocks, and I agree that constants inside blocks are definitely a risky choice and should be discouraged. I'm not convinced the author of that article knew about the < Struct.new pattern, so I wouldn't take his failure to mention it as a recommendation against it.