WIP: Add SubclassedFromCoreClass smell detector

[andyw8]

Addresses #907

Opening for early feedback. There's one pending spec, I'm still getting the hang of Parser so any advice would be welcome.

[troessner]

Looks very promising!

A couple of generic suggestions:

1.) Don't forget to require the detector in lib/reek/smells.rb
2.) An integration test would be great (that one would have also caught [1]

[troessner]

How about moving this to a new predicate method ancestor? on exp?

[andyw8]

Would that be Reek::AST::Node?

[mvz]

Those extensions are defined in modules under Reek::AST::SexpExtensions.

[andyw8]

@mvz I'm having trouble understanding how to structure this. Could you give me some guidance? (cc @troessner)

[mvz]

The modules under SexpExtensions are included in the classes representing the AST nodes. The modules that is included depends on the node type.

So nodes of type :class get ClassNode included, which is defined in lib/reek/ast/sexp_extensions/module.rb. In fact, you can see there that the relevant methods are defined in ModuleNodeBase, because many methods that are relevant for :class are also relevant for :module and :casgn nodes.

Long story short, you would probably want to define ancestor on ClassNode to start with, and then change the code above to:

  exp = ctx.exp
  return [] unless exp.ancestor

[andyw8]

I'm thinking that SubclassedFromCoreClass may not be a good name, since there are cases where subclassing a core class is the correct thing to do, e.g. with RuntimeError.

[andyw8]

@troessner re. an integration test, do you mean using Cucumber? Should I add some code containing this smell to the inline.rb sample file?

[troessner]

@troessner re. an integration test, do you mean using Cucumber? Should I add some code containing this smell to the inline.rb sample file?

No, we shouldn't touch those since they exist in the wild so this would be very confusing.
You can e.g. check out this feature to check out the current idiomatic Reek way.

[waldyr]

Sorry for blending in but I'm interested in this feature. How is that going?

[troessner]

I think @andyw8 is the only one who can answer this question at the moment ;)

@waldyr are you interested in how smell detectors are structured (i can explain that on request) or in this specific smell detector?

[waldyr]

@troessner I'm interested in this specific smell detector.

[andyw8]

Hi @waldyr, thanks for your interest. I have a lot on at the moment, but hope to get back to this next week. Do you have any thoughts on the implementation so far?

[waldyr]

@andyw8 I think the implementation is okay. Keep up with this great work :)

[troessner]

@andyw8 if you don't have the time, no problem, but let's not kill @waldyr impulse and motivation by inactivity. Let's deprecate this PR in favor of #945 - @waldyr care to over and push your PR forward?
I'll happily review and help you as soon as something is reviewable.

[troessner]

Closing this one in favor of #954 - thanks for getting this started @andyw8 !

[andyw8]

Apologies, the replies for this were filtered out of my inbox. Thanks for taking over @waldyr.