Support deep coercion for Hash attributes #112

Merged
merged 9 commits into from Oct 1, 2012

Projects

None yet

4 participants

@greyblake
Contributor

Issue #98

Here we are.

@travisbot

This pull request fails (merged 5ab13ce into 0d72026).

@travisbot

This pull request fails (merged 17a6240 into 0d72026).

@dkubb
Collaborator
dkubb commented Sep 2, 2012

@greyblake it looks like you left in a require 'pry' line in the specs, and travis is blowing up because it's not a gem dependency. I'd suggest removing that line since we don't want to add any dependencies that aren't absolutely necessary.

Also, this may be something you're already doing, but make sure you run rake ci which will run through the tests as well as some deeper checks of the code that we require the code to meet.

@travisbot

This pull request passes (merged c9e592f into 0d72026).

@greyblake
Contributor

@dkubb, thanks for catching pry. I just used it for local debug. Removed in 05bd947

rake ci allowed me to detect that one of my methods is not covered, but the output is pretty urgly:

rake ci
[warn]: in YARD::Handlers::Ruby::MixinHandler: Undocumentable mixin: YARD::Parser::UndocumentableError for class Virtus::Attribute
[warn]:     in file 'lib/virtus/attribute.rb':10:

    10: include Equalizer.new(inspect) << :name << :options

Coverage: 99.8% (threshold: 100.0%)
[warn]: in YARD::Handlers::Ruby::MixinHandler: Undocumentable mixin: YARD::Parser::UndocumentableError for class Virtus::Attribute
[warn]:     in file 'lib/virtus/attribute.rb':10:

    10: include Equalizer.new(inspect) << :name << :options

rake aborted!
Coverage must be at least 100.0% but was 99.8%

There defenetly should be a way to find out what code is not covered.
Finally with simple cov i detect it and write a spec: c4a9101

Thanks.

@dkubb
Collaborator
dkubb commented Sep 2, 2012

@greyblake yeah, you can run the yardstick_measure rake task, and then check the report in ./measurements.

Most of the output comes from parsing errors in YARD so other than silencing them I don't know if there's anything I can do in yardstick to make them go away.

@dkubb dkubb commented on the diff Sep 2, 2012
lib/virtus/attribute/hash.rb
+ # @example
+ #
+ # class Request
+ # include Virtus
+ #
+ # attribute :headers, Hash[Symbol => String]
+ # end
+ #
+ # Post.attributes[:headers].value_type # => Virtus::Attribute::String
+ #
+ # @return [Virtus::Attribute]
+ #
+ # @api public
+ attr_reader :value_type
+
+
@dkubb
dkubb Sep 2, 2012 Collaborator

It's a tiny point, but there's an extra blank line here.

@greyblake
greyblake Sep 2, 2012 Contributor

addressed in 0242d1c

@dkubb dkubb and 2 others commented on an outdated diff Sep 2, 2012
lib/virtus/attribute/hash.rb
+ @key_type_instance = Attribute.build(@name, @key_type)
+ @value_type_instance = Attribute.build(@name, @value_type)
+ end
+ end
+
+ # Coerce a hash with keys and values
+ #
+ # @param [Object]
+ #
+ # @return [Object]
+ #
+ # @api private
+ def coerce(value)
+ coerced = super
+ return coerced unless coerced.respond_to?(:inject)
+ coerced.inject(new_hash) do |hash, key_and_value|
@dkubb
dkubb Sep 2, 2012 Collaborator

You can use #each_with_object here so that you don't have to return the hash as the last statement in the block.

Don't forget to change the #respond_to? call on the line above too.

@greyblake
greyblake Sep 2, 2012 Contributor

Good idea. But it doesn't look like ruby 1.8.7 supports each_with_object method.

@solnic
solnic Sep 2, 2012 Owner

We use backports gem wich adds this method

@greyblake
greyblake Sep 2, 2012 Contributor

Addressed in: 68793d2

@dkubb dkubb and 1 other commented on an outdated diff Sep 2, 2012
lib/virtus/attribute/hash.rb
+ # keys and values on assigning.
+ #
+ # @return [Hash]
+ #
+ # @api private
+ def new_hash
+ hash = self.class.primitive.new
+ return hash unless @key_type_instance && @value_type_instance
+
+ key_coercion_method = @key_type_instance.coercion_method
+ value_coercion_method = @value_type_instance.coercion_method
+
+ # Redefine []= method to coerce key and value on assigning.
+ # It requires inlining of Attribute#coerce method to coerce.
+ # An alternative way would be using define_singleton_method or Sinatra's meta_def.
+ hash.instance_eval(<<-eorb, __FILE__, __LINE__+1)
@dkubb
dkubb Sep 2, 2012 Collaborator

This does some bad things to the method cache in MRI. Is there another way we can do it without extending objects on the fly like this?

Maybe we can use a proxy object that wraps the Hash instead? A Hash subclass would also work, and might be better.

@greyblake
greyblake Sep 2, 2012 Contributor

Has a sense. Should we cache hash proxy classes with key_type and value_type as keys to reduce number of anonymous classes? Or create them dynamically for every new instance of hash?

@greyblake
greyblake Sep 15, 2012 Contributor

I removed redefenition of []= since you said it's out of scope.
01922f8

@greyblake
Contributor

@dkubb To avoid yardstick warnings we can extract module created dynamically into a constant:

    # Some doc
    AttributeEalizer = Equalizer.new(inspect) << :name << :options
    include AttributeEalizer
@travisbot

This pull request passes (merged 0242d1c into 0d72026).

@travisbot

This pull request passes (merged 68793d2 into 0d72026).

@greyblake
Contributor

@dkubb @solnic Does something remain to do to merge this pull request?

@solnic
Owner
solnic commented Sep 25, 2012

Yes. Me or @dkubb must find time to do the final review and merge :) I'll work on it when I'm in Kiev next weekend :) conferences == OSS hacking :)

On Tuesday, September 25, 2012 at 3:00 PM, Sergey Potapov wrote:

@dkubb (https://github.com/dkubb) @solnic (https://github.com/solnic) Does something remain to do to merge this pull request?


Reply to this email directly or view it on GitHub (#112 (comment)).

@greyblake
Contributor

Ok.
Btw, I look forward to your talk in Kiev and hope I'll catch you there:)

@solnic solnic merged commit 01922f8 into solnic:master Oct 1, 2012

1 check failed

default The Travis build failed
Details
@solnic
Owner
solnic commented Oct 1, 2012

@greyblake thanks for that. I just merged it in :)

@greyblake
Contributor

Great! Thanks))

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment