Not possible to create anonymous classes to create boxed attribute types

[solnic]

Not possible to create anonymous classes to create boxed attribute types

[d11wtq]

Dan has fixed this :)

[dkubb]

This was fixed in 86c0dc2

@solnic wdyt about a 0.0.9 release with this fix?

[d11wtq]

+1 for a 0.0.9 release (because I'm selfish and need it :P)

[solnic]

sure but first we need to clean up that type lookup a bit cause our metric tools aren't happy:

lib/virtus/support/type_lookup.rb -- 3 warnings:
  Virtus::TypeLookup#determine_type_from_primitive calls descendant.primitive twice (Duplication)
  Virtus::TypeLookup#determine_type_from_primitive refers to descendant more than self (LowCohesion)
  Virtus::TypeLookup#determine_type_from_primitive refers to type more than self (LowCohesion)

and

lib/virtus/support/type_lookup.rb:69 - Block cyclomatic complexity is 4.  It should be 2 or less.

[d11wtq]

Virtus::TypeLookup#determine_type_from_primitive calls descendant.primitive twice (Duplication)

This looks fine to me. It's not duplication of logic, it's just avoiding temporary variables (a code smell itself):

    def determine_type_from_primitive(primitive)
      type = nil

      descendants.reverse_each do |descendant|
        next unless primitive <= descendant.primitive
        type = descendant if type.nil? || type.primitive > descendant.primitive
      end

      type
    end

Virtus::TypeLookup#determine_type_from_primitive refers to descendant more than self (LowCohesion)
Virtus::TypeLookup#determine_type_from_primitive refers to type more than self (LowCohesion)

Again, the only way I see to fix those is to copy descendant.primitive and type.primitive into a temporary variable, which, IMHO is not something that should be encouraged. Unnecessary temporary variables make refactoring harder further down the line.

I'm not seeing the same warning about the cyclomatic complexity. I assume somebody already changed this?

[solnic]

@d11wtq I tried to quickly simplify this method and failed. this isn't blocking the release of course, so if we can't come up with a better implementation now I will release 0.0.9 so the bug that affects you is gone and do some small refactoring for 0.1.0. I'll push 0.0.9 later today.

[d11wtq]

You could define methods type_primitive and descendant_primitive, but that doesn't really simplify anything and only serves to trick reek ;)

[d11wtq]

PS: Thanks for considering me in your release schedule! I can't give enough praise to Virtus. I love it. Makes coding against a remote API way more fun :P

[dkubb]

One refactoring for the future would be to move the responsibility of making the "best match" into the descendants object.

Currently the descendants list is an Array, because I didn't need anything more complex, but given that other parts of the project are using descendants, perhaps it's time to make a minimally complete object that can be appended to, and knows how to match a class within itself. I'm not too comfortable exposing something important as part of an API that's made up solely of primitive objects.

I might also consider making a method either on the Descendants or Attribute that knows how to do the "best match"; it will either return the current Attribute object, or the one provided, whichever is more specific.

[dkubb]

I should add too that I don't think such a refactoring should be a release blocker.