madeintandem / hstore_accessor

Adds typed hstore-backed field support to ActiveRecord models.
MIT License
242 stars 47 forks source link

extend the getters and setters #19

Closed andreorvalho closed 10 years ago

andreorvalho commented 10 years ago

My problem is I have an array field has an hstore accessor and when I present it on a form the value include the empty string(rails default behaviour), for example: ["", "1", "2"]. And for obvious reasons I would like to save it without the "". So I have tried:

class Standard < ActiveRecord::Base
  # Extensions
  hstore_accessor :properties, specific_content: :array

  def specific_content=(value)
    value = value.reject!(&:blank?)
    super(value)
  end
end

First I get an undefined method `reject!' for nil:NilClass, so I was getting the impression this was called with the wrong value, but I could see when I printed the values that it was first called with ["", "1", "2"] and the with nil. So I have tried and add a value = value.reject!(&:blank?) if value and this gives a stack level too deep and I can see that the value is printed infinitely.

So my conclusion is I cant extend the setter with something I need to do before it's called, since it seems to override what hstore_accessor is doing!

I can see here: https://github.com/devmynd/hstore_accessor/blob/master/lib/hstore_accessor/macro.rb#L25 that's where the definition happens. Do you think it's possible to change it to allow me to do something like above? Or should I be doing this in another way?

crismali commented 10 years ago

You can achieve the same thing with something like this:

class Standard < ActiveRecord::Base
  # Extensions
  hstore_accessor :properties, specific_content: :array

  alias_method :set_specific_content, :specific_content=

  def specific_content=(value)
    value = value.reject!(&:blank?)
    set_specific_content(value)
  end
end

I'd agree that being able to call super would be more convenient though.

thegrubbsian commented 10 years ago

I agree with @crismali, I think his solution might be the only way to do this. You can't actually call super in this context since there really is no superclass method named specific_content=. When the Macro module is included it actually defines the methods directly on the class, they aren't brought in from a module so they don't have a super.

The only other thing I can think of would be to provide a hook method that you could define, something like:

# before_<hstore name>_<field name>_set
def before_properties_specific_content_set(value)
  value.reject!(&:blank?)
end

This would get called by the setter right before it did it's work, the return value of the hook method would be the updated value pushed into the hstore field.

andreorvalho commented 10 years ago

@crismali Thanks for your suggestion is definitely better than what I add with before_validation callbacks.

@thegrubbsian Why not using @crismali suggestion to be actually implemented dinamically. so something like(using the macro.rb definitions):

  alias_method "set_#{key}=".to_sym, "#{key}=".to_sym

  define_method("#{key}=") do |value|
    send("set_#{key}=".to_sym, value)
  end

Then we could actually just define it like this:

class Standard < ActiveRecord::Base
  # Extensions
  hstore_accessor :properties, specific_content: :array

  def specific_content=(value)
    value = value.reject!(&:blank?)
    super(value)
  end
end

I am not so sure this could actually work, as I am not sure why does the alias_method above works :)

But what do you guys think?

andreorvalho commented 10 years ago

just a little comment:

we can't use

value.reject!(&:blank?)

since it returns nil, it needs to be

value.reject(&:blank?)
thegrubbsian commented 10 years ago

That still doesn't really work based on the way we're building the macro methods. @crismali had a good idea about dynamically building a module that would then get mixed in to the class by the macro, that would give you access to a super method. We'll look into that and see if it makes sense.

crismali commented 10 years ago

Here's an implementation of that idea: https://github.com/devmynd/hstore_accessor/pull/21

I'm open to other ways to test that kind of functionality :)

andreorvalho commented 10 years ago

@crismali I have commented there, but besides that it looks perfect to solve my problem :)