Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

Coercion for arrays doesn't work with `<<` method. #114

Closed
greyblake opened this Issue Sep 10, 2012 · 4 comments

Comments

Projects
None yet
3 participants
Contributor

greyblake commented Sep 10, 2012

Just detected, testing the previous bug:

class BitMask
  include ::Virtus
  attribute :bits, Array[Integer]
end

bit_mask = BitMask.new
bit_mask.bits                    # => [], OK
bit_mask.bits = ["0", 3, "5"]
bit_mask.bits                    # => [0, 3, 5],  OK, coercion works
bit_mask.bits << "7"
bit_mask.bits                    # => [0, 3, 5, "7"], Ooops

There is no rush, I can fix it myself on weekends.

Contributor

greyblake commented Sep 10, 2012

Finally we need to return a subclass of Array with redefined << method, like with Hash

Collaborator

dkubb commented Sep 10, 2012

This is a slippery slope. You would have to subclass Array and override every mutator method to coerce the new object. It would have to be kept up to date as the interfaces change, and make sure to not break any interfaces in the meantime. I've done this kind of thing before in DataMapper, and that was just for Array, and it is a lot of work.

I think this is outside of what virtus should be doing. If someone wants to make an extension to virtus to allow mutation to be coerced, that's fine, but it probably shouldn't be in core.

@solnic wdyt?

Contributor

greyblake commented Sep 11, 2012

@dkubb, you have a sense, it requires tracking of every mutation method. As I know you are guys going to split virtus(into core, coercions, etc). I believe this part of functionality will go to coercions.
On other hand syntax like Array[SomeType] and Hash[Type1 => Type2] just confuse people when it's meant to do coerce only on assignment.

IMHO, I guess we can start fixing those bugs and then when the library will be split we'll move it to "coercion" part.

Owner

solnic commented Sep 11, 2012

I was going to comment on the PR with Hash changes that it's a bit outside of the virtus' scope. As @dkubb mentioned we tried tackling that problem in DM1 and solutions aren't trivial. We can experiment in a plugin and maybe in the future when we have a good way of solving this we could merge it into virtus.

At the moment the contract is simple - virtus performs coercion on value assignment. Any further mutations of a value that are handled by the value's class API (as in Array#<< etc.) are not going through virtus coercion system so people shouldn't expect coercions to kick in.

@solnic solnic closed this in 4ef7e85 Oct 1, 2012

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