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

ABC violation output is weird for classes defined using Struct.new do...end #20

Closed
myronmarston opened this issue Aug 13, 2012 · 1 comment · Fixed by #24
Closed

ABC violation output is weird for classes defined using Struct.new do...end #20

myronmarston opened this issue Aug 13, 2012 · 1 comment · Fixed by #24

Comments

@myronmarston
Copy link
Contributor

I've got a class defined like so:

module Packrat
  CollectionContainer = Struct.new(:collection_occurrence) do
    def metadata
      # some code that violates my ABC threshold
    end
  end
end

The output from the ABC check is:

Methods exceeded maximum allowed ABC complexity (1):

  lib/packrat/structs.rb  Packrat > metadata  12

Notice that the method is listed as Packrat > metadata rather than Packrat > CollectionContainer > metadata. I can easily see why: I'm defining the method in the block passed to Struct.new, and cane doesn't realize that it's being defined on the CollectionContainer class since it's not the normal class CollectionContainer ... end style class definition.

It'd be reasonable to simply say "this isn't supported by cane", but if it wasn't too difficult, it'd be nice. I prefer defining struct classes using this block form over class CollectionContainer < Struct.new because the latter form subclasses a subclass of Struct (since Struct.new returns a Struct subclass) and therefore has an extra class in the ancestor chain, which can slow down method dispatch a bit by giving ruby an extra stop on the list of ancestors it traverses when dispatching methods.

If something is done to handle this case, it can be applied to defining classes using Class.new { ... } as well, since it's basically the same issue.

@xaviershay
Copy link
Contributor

Good call. I'll have a poke around and see if there's a decent fix.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants