Coercion type systems #379

mkcode · 2016-10-25T19:59:18Z

Introducing switchable type coercion systems for Hashie. Follows up on #239.

Adds 'dry-types' and 'activemodel' alongside Hashie's builtin coercion system. Basic usage is written up in the README.

For the most part, reading the source should be straight forward. The 'coercion' folder under 'extensions' contains the implementation of the 3 coercion systems. The Hashie::Extensions::Coercion module was split into two files. The file itself contains the interface and common coercion methods, while the Coercion::HashieTypes class now contains the actual coercion implementation.

The only confusing part is the CoercionSystemIncludeBuilder defined under Hashie::Extensions::Coercion. The purpose of this class is to DRY up the included hooks for Hashie::Extensions::Coercion and Hashie::Extensions::Dash::Coercion. The common includes are defined once, and reused in the default included method and the active_model and dry_types methods on both the default and Dash coercion classes. The active_model and dry_types methods on Hashie::Extensions::Coercion and Hashie::Extensions::Dash::Coercion both return an anonymous module with an included method correctly defined, which then is immediately executed because it then included.

Example:

mod = Hashie::Extensions::Coercion.dry_types
# => <Module: xyz>

mod.methods
#=> [:included]

class MyHash < Hashie::Hash
  include mod
end
#=> Hashie::Extensions::Coercion::ClassMethods, Hashie::Extensions::Coercion::InstanceMethods, and Hashie::Extensions::Coercion::DryTypes are all mixed into MyHash

This is 100% backwards compatible and all the tests pass. I did not add any additional tests - we should not need many as the new coercion systems are well tested in their own repos. We may want to add a few tests though.

broke out the builtin Hashie type coercions into a system that is able to load different type coercion systems in a configurable way. The new CoercionSystemIncludeBuilder creates anonymous modules with `included` methods that will correctly include the overall coercion modules and the sub type system override modules into the base class. It also is reused in the `Dash::Coercion` class, so that we do not need to repeat any code there. Default behavior of running `include Hashie::Extensions::Coercion` retains the same behavior to make this a non-breaking change.

ActiveModel::Type is new as of version 5. Types were defined in ActiveRecord in rails 4.2. We need to update activesupport to version 5 to be work along side ActiveModel v5. Thankfully, no breaking changes.

…uder

this is by far the nicest type system and easiest to implement

mkcode · 2016-10-25T20:13:34Z

Ah, and I now see that version 5 of the active* gems require recent rubies. I'm much more in favor of getting dry-types in rather than the activemodel types. So two ways to proceed: We can either make the active model system work with older rubies or just remove it. Maintainers choice. :-)

This now works with older rubies as well.

…fined

dblock · 2016-10-27T08:11:13Z

Gemfile

@@ -12,7 +12,18 @@ end

 group :test do
  # ActiveSupport required to test compatibility with ActiveSupport Core Extensions.
-  gem 'activesupport', '~> 4.x', require: false
+  if RUBY_VERSION >= '2.2.2'


Theoretically you're not allowed to compare like this. Use the hashie extension from #375, RubyVersion.new(RUBY_VERSION) >= RubyVersion.new(...).

dblock · 2016-10-27T08:11:40Z

README.md

@@ -30,6 +30,54 @@ Any of the extensions listed below can be mixed into a class by `include`-ing `H

 ### Coercion

+Three systems are available for coercion: ActiveModel types, dry-types, and hashie's builtin types. The use of a specified coercion system is determined by exactly which Coercion module you include in your Hashie class. Including the default `Hashie::Extensions::Coercion` module will use Hashie's builtin type system.


I think it's built-in.

I would lowercase which Coercion ... here.

I think links to the other two libraries' source would be good here too. [ActiveModel](https://github.com/rails/rails/tree/master/activemodel) and [dry-types](https://github.com/dry-rb/dry-types).

Also, uppercase Hashie please.

dblock · 2016-10-27T08:14:06Z

README.md

+
+### Coercion using ActiveModel types
+
+To use ActiveModel's type coercion system, you must add version 5 of ActiveModel to your Gemfile. If you are using Rails, activemodel is included, but you must be using Rail's version 5 in order to use this. If you are on running a lower version of Rails, you will not be able to use ActiveModel's type coercion system due to Ruby's inability to use different Gem versions in the same application.


Is this still true? It sounds quite confusing, I didn't know you could use different gem versions in the same application for example :) I think Hashie shouldn't be concerned too much about explanations of why and focus on what. I'd leave the Rails stuff out too, and just worry about activemodel.

I didn't know you could use different gem versions in the same application for example :)

Right, Ruby cannot use different versions in the same app. I did say exactly this. Ok, I'll remove the rails related info.

I see why it's confusing now ;)

You're saying: if you cannot add activemodel 5 to Gemfile you cannot use the activemodel coercion system. Well, that's obvious, no? Just say that in order to use it you need AM 5 and call it a day? It will also be future proof, who knows what happens with AM 5.1 or whatever next version, we shouldn't be in the business of telling users how to install someone else's gem too much.

I totally agree with dB here.

Ok. I'll update these instructions to what you mentioned.

dblock · 2016-10-27T08:14:36Z

README.md

+
+```ruby
+class Tweet < Hash
+  include Hashie::Extensions::Coercion.active_model


It's a bit weird that this is not Hashie::Extensions::Coearcion::ActiveModel, don't you think?

I actually do not find it weird and I think it makes more sense than a constant because it conveys that this returns an anonymous module. Plenty of gems do use this style, such as dry-types: http://dry-rb.org/gems/dry-types/including-types/ and virtus: https://github.com/solnic/virtus#using-virtus-with-classes.

That is the argument for it and why I set it up this way. Let me know if you may have been persuaded :-). If not, no problem and I'll update this to use constants.

I see. I'd say for Hashie the reason to use a class is because Hashie typically includes all extensions that way. I can be swayed both ways, maybe someone else will have an opinion like @michaelherold?

Problems with dynamic modules

The problem I see with doing anonymous module building is that when you're trying to debug an issue, it gets harder to look at where a method comes from when it shows up in Stuff.ancestors as a <Module#...> rather than <Hashie::Extensions::Coercion::ActiveModel>.

I thought the dynamic module idea was interesting a while ago (in fact, I have an in progress feature for Hashie using a framework I built for building modules that uses this technique) but ran up against that problem.

Do we see that as an issue?

Reason for approach

Secondly, is the reason for this approach just to eliminate a little bit of duplicate code? I feel like it can actually make it harder for follow what's going on in the source, which makes maintenance harder down the road. If it's just for that purpose, I think it would be better to just be explicit and not rely on metaprogramming.

If there's a more in-depth reason, I'm all ears!

@michaelherold - I totally agree with everything you are saying about dynamic modules. What you a saying does not apply here though. When I said that dry_types returns a anonymous module, that anonymous module only has an included block defined and lives for a split second as it is being included into the base class. The modules that are mixed into the base class are static, so pry's ls method and the ancestors method are just as descriptive as before.

mod = Hashie::Extensions::Coercion.dry_types # => <Module: xyz> mod.methods #=> [:included] class MyHash < Hashie::Hash include mod end #=> Hashie::Extensions::Coercion::ClassMethods, Hashie::Extensions::Coercion::InstanceMethods, and Hashie::Extensions::Coercion::DryTypes are all mixed into MyHash

Your second point is right. The primary reason for this is to remove duplicate code. Now that I explained that the first point is not an issue, perhaps code cuplication that is removed is more valuable? This is a tradeoff, and it certainly is easy to switch to simple way with code duplication. I should note that if we go this route, the common include block which mixes in the base Hashie::Coercion ClassMethods and InstanceMethods will need to duplicated 6 times throughout the code. Once for every coercion system type in both the Coercion and Dash::Coercion extension modules.

Please let me know your thoughts and instruct. :-)

dblock · 2016-10-27T08:14:52Z

README.md

+[:big_integer, :binary, :boolean, :date, :datetime, :decimal, :float, :immutable_string, :integer, :string, :text, :time, :symbol]
+```
+
+The `cast` or `cast_value` methods on the `ActiveModel::Type` class are used, `serialize` and `deserialize` are for database interaactions and may be ignored in this context. Read more about ActiveModel::Type here: https://github.com/rails/rails/tree/5-0-stable/activemodel/lib/active_model/type


Missing a period at the end.

interaactions -> interactions.

Also, I'd rather see the docs link as: [See the ActiveModel::Type documentation for more information](https://github.com/rails/rails/tree/5-0-stable/activemodel/lib/active_model/type).

dblock · 2016-10-27T08:15:17Z

README.md

+end
+```
+
+Dry-types has far too many types and type constraints available to list here. Please read the documentation to learn what is available: http://dry-rb.org/gems/dry-types/built-in-types/.


Make links consistent, ie. without a foward slash at the end.

dblock · 2016-10-27T08:15:55Z

README.md

+
+Dry-types has far too many types and type constraints available to list here. Please read the documentation to learn what is available: http://dry-rb.org/gems/dry-types/built-in-types/.
+
+### Coercion


We now have two coercion sections, this one should be something else, like Coercion Rules and be under Coercion, or the other should be Coercion Systems?

I'll update the other to be Coercion Systems.

dblock · 2016-10-27T08:16:38Z

lib/hashie/extensions/coercion.rb

+          define_include_type_method base, :dry_types do |base_class|
+            base_class.extend DryTypes
+          end
+          return unless RUBY_VERSION >= '2.2.2'


Use comparison method from Extensions, see above.

dblock · 2016-10-27T08:18:11Z

lib/hashie/extensions/coercion/active_model.rb

+  module Extensions
+    module Coercion
+      module ActiveModel
+        # Symbol is the one glaring type omission. Define it if it is not


Is this a version thing? I mean is symbol going to be defined in the future? Feels weird to be defining a type within ActiveModel here, it should be external to here and probably not be in the ActiveModel namespace, but in Hashie's.

This already is Hashie's namespace: Hashie::Extensions::Coercion::ActiveModel. Anything to do here?

I believe there is no symbol type because the focus of ActiveModel::Type is interop with DBs - none of which use symbols.

Yes, this comment was related to what's below. I find the comment less confusing having read the code, but really the dynamic class definition at runtime is my issue here.

dblock · 2016-10-27T08:19:56Z

This looks like an amazing start, I definitely want to see this finished.

My only major gripe is that to use a coercion system you have to include something other than a module. I think it should be Hashie::Extensions::Coercion::DryTypes and not Hashie::Extensions::Coercion.dry_types for example.

Of course it needs tests big time :)

mkcode · 2016-10-28T01:29:48Z

@dblock - ✨ Thanks so much for being receptive to this update and providing such detailed feedback. I'll address all the points you mentioned in the next few days and we'll have another go at it!

michaelherold

I'm liking the idea of this so far. I've only got through a bit of the code and would love to give it more time, but I'm heading out to go hiking right now.

I left some comments where I've looked and will come back when I get home!

michaelherold · 2016-10-28T15:41:23Z

Gemfile

@@ -12,7 +12,18 @@ end

 group :test do
  # ActiveSupport required to test compatibility with ActiveSupport Core Extensions.
-  gem 'activesupport', '~> 4.x', require: false
+  if RUBY_VERSION >= '2.2.2'
+    gem 'activesupport', '~> 5.x', require: false


Hmm, this is a new bundler syntax to me. I haven't seen a .x version before. Is this the same as ~> 5?

I think ~> 5 means anything > 5, but ~> 5.x means any 5.x version.

michaelherold · 2016-10-28T15:44:24Z

README.md

@@ -30,6 +30,54 @@ Any of the extensions listed below can be mixed into a class by `include`-ing `H

 ### Coercion

+Three systems are available for coercion: ActiveModel types, dry-types, and hashie's builtin types. The use of a specified coercion system is determined by exactly which Coercion module you include in your Hashie class. Including the default `Hashie::Extensions::Coercion` module will use Hashie's builtin type system.


I think links to the other two libraries' source would be good here too. [ActiveModel](https://github.com/rails/rails/tree/master/activemodel) and [dry-types](https://github.com/dry-rb/dry-types).

Also, uppercase Hashie please.

michaelherold · 2016-10-28T15:45:13Z

README.md

+
+### Coercion using ActiveModel types
+
+To use ActiveModel's type coercion system, you must add version 5 of ActiveModel to your Gemfile. If you are using Rails, activemodel is included, but you must be using Rail's version 5 in order to use this. If you are on running a lower version of Rails, you will not be able to use ActiveModel's type coercion system due to Ruby's inability to use different Gem versions in the same application.


I totally agree with dB here.

michaelherold · 2016-10-28T15:52:47Z

README.md

+
+```ruby
+class Tweet < Hash
+  include Hashie::Extensions::Coercion.active_model


Problems with dynamic modules

The problem I see with doing anonymous module building is that when you're trying to debug an issue, it gets harder to look at where a method comes from when it shows up in Stuff.ancestors as a <Module#...> rather than <Hashie::Extensions::Coercion::ActiveModel>.

I thought the dynamic module idea was interesting a while ago (in fact, I have an in progress feature for Hashie using a framework I built for building modules that uses this technique) but ran up against that problem.

Do we see that as an issue?

Reason for approach

Secondly, is the reason for this approach just to eliminate a little bit of duplicate code? I feel like it can actually make it harder for follow what's going on in the source, which makes maintenance harder down the road. If it's just for that purpose, I think it would be better to just be explicit and not rely on metaprogramming.

If there's a more in-depth reason, I'm all ears!

michaelherold · 2016-10-28T15:55:08Z

README.md

+[:big_integer, :binary, :boolean, :date, :datetime, :decimal, :float, :immutable_string, :integer, :string, :text, :time, :symbol]
+```
+
+The `cast` or `cast_value` methods on the `ActiveModel::Type` class are used, `serialize` and `deserialize` are for database interaactions and may be ignored in this context. Read more about ActiveModel::Type here: https://github.com/rails/rails/tree/5-0-stable/activemodel/lib/active_model/type


interaactions -> interactions.

Also, I'd rather see the docs link as: [See the ActiveModel::Type documentation for more information](https://github.com/rails/rails/tree/5-0-stable/activemodel/lib/active_model/type).

mkcode · 2016-10-28T18:52:08Z

@michaelherold - ✨ And thanks for taking the time for a detailed review. Looking forward to getting more feedback when you get back from hiking.

dblock · 2016-10-30T09:07:44Z

Hiking? How do you code while hiking @mkcode ??? :) Enjoy!

michaelherold

First off, I'd like to say thank you for taking the time to submit this! It looks like you've put quite a bit of time into it so far.

I left a bunch of comments where I thought the code was confusing or where I saw something that I thought would be an improvement. It's looking pretty solid so far!

I would really like to see this refactored into an actual adapter pattern rather than the pseudo-adapter that it currently is. I think it will help clean up some repetition and perhaps make it so we don't have to rely on dynamically generating modules that do mostly the same thing. It will also create an extension point for people to write their own adapters, which would be great!

This might be a tough refactor, though, so let's do a few things first.

I think there are some straightforward next steps:

Add tests. Writing tests that show the expected behavior will let us refactor the code without breaking the functionality.
Document all of the public methods (and if you're feeling adventurous, the private methods too!). This is something that we're trying to be better at as a project. I have a slight preference for YARD, but there's an open issue for voting on a format that you might want to weigh in on.

Once those are done, I'll be better able to assist in extracting the commonalities to make a friendlier interface and clean up a little bit.

michaelherold · 2016-10-31T22:22:03Z

lib/hashie/extensions/coercion.rb

-      def self.included(base)
-        base.send :include, InstanceMethods
-        base.extend ClassMethods # NOTE: we wanna make sure we first define set_value_with_coercion before extending
+      class CoercionSystemIncludeBuilder < Module


I'd like this extracted to its own file at lib/hashie/extensions/coercion/coercion_system_include_builder.rb.

Also, why is this a class that inherits from Module? Shouldn't it really be one or the other?

michaelherold · 2016-10-31T22:24:58Z

lib/hashie/extensions/coercion.rb

-            return value.send(name)
-          end
+        def build_coercion
+          fail 'This method must be replaced'


I think this should be something along the lines of this:

fail NotImplementedError, "Your coercion adapter has not implemented the `#build_coercion` method."

That helps with tracking down why the error is being thrown. Perhaps adding what adapter they're currently using would be good as well.

michaelherold · 2016-10-31T22:27:27Z

lib/hashie/extensions/coercion/active_model.rb

+        end
+
+        # Array of symbols of the included ActiveModel types.
+        ACTIVE_MODEL_TYPES = ::ActiveModel::Type.registry


Reformatting this to look like the following can help with readability:

ACTIVE_MODEL_TYPES = ::ActiveModel::Type .registry .send(:registrations) .map { |r| r.send(:name) } .freeze

Also, please use #__send__ instead of #send. Since we're a library, we have to gracefully handle people doing strange things like overriding the #send method.

Lastly, why .send(:registrations) and not just .registrations?

michaelherold · 2016-10-31T22:31:55Z

lib/hashie/extensions/coercion/active_model.rb

+                             .map { |r| r.send(:name) }
+                             .freeze
+
+        def build_coercion(type)


This is a very long method. I think it would help understanding to break it apart at each case:

def build_coercion(type) if type.is_a? Enumerable build_enumerable_coercion(type) elsif ACTIVE_MODULE_TYPES.include? type build_active_model_coercion(type) # ... end end

Ideally, I would like to see this class broken up a little more (i.e. into single responsibility objects that know how to create a coercion of each type), but we can work on that after we hammer everything else out.

michaelherold · 2016-10-31T22:33:58Z

lib/hashie/extensions/coercion/active_model.rb

+        def build_coercion(type)
+          if type.is_a? Enumerable
+            if type.class <= ::Hash
+              type, key_type, value_type = type.class, *type.first


This is a little confusing. I think this is the same and is a little clearer? I find the splat to still be confusing.

key_type, value_type = *type.first build_hash_coercion(type.class, key_type, value_type)

michaelherold · 2016-10-31T22:44:27Z

lib/hashie/extensions/coercion/dry_types.rb

+        def build_coercion(type)
+          if type.respond_to? :call
+            lambda do |value|
+              return type.call(value)


Unless it's necessary, let's elide the return.

michaelherold · 2016-10-31T22:45:28Z

lib/hashie/extensions/coercion/dry_types.rb

+            end
+          elsif type.respond_to? :coerce
+            lambda do |value|
+              return value if value.is_a? type


See my comments about the other "lamdbas that just return the original". I think we can just coerce.

michaelherold · 2016-10-31T22:45:35Z

lib/hashie/extensions/coercion/dry_types.rb

+              type.coerce(value)
+            end
+          elsif type.respond_to? :new
+            lambda do |value|


See my comments about the other "lamdbas that just return the original". I think we can just call new.

michaelherold · 2016-10-31T22:46:28Z

lib/hashie/extensions/coercion/hashie_types.rb

+        }
+
+        def build_coercion(type)
+          if type.is_a? Enumerable


This looks like duplicated code.

I'd love it if we could figure out a way to get rid of the duplication.

michaelherold · 2016-10-31T22:48:09Z

lib/hashie/extensions/dash/coercion.rb

-
-        def self.included(base)
-          base.send :include, Hashie::Extensions::Coercion
+        includer = ::Hashie::Extensions::Coercion::CoercionSystemIncludeBuilder


I thought we could still use Hashie::Extensions::Coercion like we used to for backward compatibility?

I think I'd rather keep the backward compatibility in Dash. What do you think @dblock?

I'm down with backward compatibility in general, but I am not sure what the implications are here.

mkcode added 11 commits October 25, 2016 02:42

update activesupport to v5

0102358

ActiveModel::Type is new as of version 5. Types were defined in ActiveRecord in rails 4.2. We need to update activesupport to version 5 to be work along side ActiveModel v5. Thankfully, no breaking changes.

add activemodel to Gemfile

a99375d

add activemodel coercion system

2dc927c

reuse Extension::Coercion common include block in Dash::Coercion incl…

b74901d

…uder

add dry-types to Gemfile

f394f53

add dry-types coercion system

8abbcff

this is by far the nicest type system and easiest to implement

move Types constant setting to own extended block

d4c2edd

add type systems documentation to Gemfile

f7e3718

fix rubocop offenses

6b233e5

add CHANGELOG entry

d554953

mkcode added 9 commits October 25, 2016 16:34

fix ruby versions prior to 2.2.2

39e91d8

fix rubies prior to 2.1.0

9f554be

fix old rubies again

1480d77

add back other user coercions to dry-types

d0270ac

add back other user coercions to active_model

d296a25

only add the symbol type to ActiveModel::Type if it is not already de…

1af6fc6

…fined

Allow for explicit Coercion.hashie_types method

201b179

include Dash example in README

59edcef

use guard clause to appease rubocop

cbbfb21

dblock reviewed Oct 27, 2016

View reviewed changes

michaelherold reviewed Oct 28, 2016

View reviewed changes

michaelherold reviewed Oct 31, 2016

View reviewed changes

michaelherold added the new feature label Oct 31, 2016

dblock force-pushed the master branch from 4a69c4a to e9c5771 Compare November 8, 2021 03:15

		@@ -30,6 +30,54 @@ Any of the extensions listed below can be mixed into a class by `include`-ing `H

		### Coercion

		Three systems are available for coercion: ActiveModel types, dry-types, and hashie's builtin types. The use of a specified coercion system is determined by exactly which Coercion module you include in your Hashie class. Including the default `Hashie::Extensions::Coercion` module will use Hashie's builtin type system.


		### Coercion using ActiveModel types

		To use ActiveModel's type coercion system, you must add version 5 of ActiveModel to your Gemfile. If you are using Rails, activemodel is included, but you must be using Rail's version 5 in order to use this. If you are on running a lower version of Rails, you will not be able to use ActiveModel's type coercion system due to Ruby's inability to use different Gem versions in the same application.


		Dry-types has far too many types and type constraints available to list here. Please read the documentation to learn what is available: http://dry-rb.org/gems/dry-types/built-in-types/.

		### Coercion

Coercion type systems #379

Are you sure you want to change the base?

Coercion type systems #379

Conversation

mkcode commented Oct 25, 2016 • edited

mkcode commented Oct 25, 2016 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dblock Oct 27, 2016 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dblock Oct 28, 2016 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mkcode Oct 28, 2016 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mkcode Oct 28, 2016 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mkcode Oct 28, 2016 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dblock commented Oct 27, 2016

mkcode commented Oct 28, 2016

michaelherold left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mkcode commented Oct 28, 2016

dblock commented Oct 30, 2016

michaelherold left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mkcode commented Oct 25, 2016 •

edited

mkcode commented Oct 25, 2016 •

edited

dblock Oct 27, 2016 •

edited

dblock Oct 28, 2016 •

edited

mkcode Oct 28, 2016 •

edited

mkcode Oct 28, 2016 •

edited

mkcode Oct 28, 2016 •

edited