sxross / MotionModel

Simple Model and Validation Mixins for RubyMotion
MIT License
192 stars 67 forks source link

Added write_attribute method for attribute setter methods #120

Closed cognitiveflux closed 9 years ago

cognitiveflux commented 9 years ago

Allows similar use of 'write_attribute' in ActiveRecord for methods that override an attribute setter Same as ActiveRecord, write_attribute does not save the model instance

sxross commented 9 years ago

Would it be beneficial if the write_attribute fired a notification when the change occurred?

cognitiveflux commented 9 years ago

In the case of write_attribute I don't think so because as in the case for ActiveRecord, this method will just set the attribute but it will not save the model. As soon as the model is saved, the normal notification is fired.

I noticed in the set_attr method it will call a defined setter method, but there is no method available (that I noticed at least) to update an attribute before saving without knowing the internals of the MotionModel data structure and flow, hence I thought write_attribute provides a sufficient abstraction:

...
def some_attribute=(value)
   # Optionally do something to 'value'
   write_attribute(:some_attribute, value)
end
...

This all being said, if there was an update_attribute, just like in ActiveRecord, this should fire a notification since it would be saving the record, but I believe calling save will do this anyhow, correct?

sxross commented 9 years ago

Calling save would have different semantics from write_attribute and then save because validations (and any casting that needed doing) would be bypassed.

Consider:

columns foo: :integer
...
my_model_obj.foo = '1'
my_model_obj.save
# Validations and casting take place, foo is an integer, 1

my_model_obj.foo.write_attr ['1', '2', '3']
my_model_obj.save
# What is the intended behavior here?

The logic behind update_attribute in Rails, if I understand correctly, is to constrain the amount of SQL you use and optimize the use of the database's execution plan. And it, too, bypasses validations.

I guess I'm not sure why you want to modify a given attribute directly and that's what is causing me to ask these questions.

cognitiveflux commented 9 years ago

It was really my intention to provide a cleaner abstraction for a custom setter, otherwise why even dispatch attribute assignment to a defined method here?

I didn't see a mention in the documentation of how best to implement a custom setter method. How else would you set the attribute value aside from doing @data[:attribute] = value in the setter?

In some cases, defining a method setter can provide more detailed validations or data manipulation. Not the best example, but consider:

class UserProfile
...
def username=(value)
   name = value.empty? ? 'Guest' : value.downcase
   write_attribute(:username, name)
end
...
end

In the case of the above, calling UserProfile.create(args) will create and save the supplied data, formatting the username attribute as defined.

Defining a setter method also allows bypassing the type casting if a different behavior is needed, as is the case for :date types (which is why I also created #121). In the case of that pull request, the :date type cast was not parsing the date field correctly because as static date format was defined, which to be fair is more of a feature request, hence the other pull request.

sxross commented 9 years ago

Ok, got it. I guess the issue of custom setters has never arisen before.

cognitiveflux commented 9 years ago

@sxross if this isn't going to be merged, is the method within the pull request the recommended way of modifying an attribute during bulk assignment prior to being saved? For example:

@data[attr_name] = value
@dirty = true

I know this is dealing with the internals of MotionModel, that's why I'm asking for clarification.

sxross commented 9 years ago

This merge ignores #121, but does enable the manual setting of a :date type, if I understand correctly. The code in DateParser was designed to handle the common cases, and absent something like the Chronic gem, adding a bunch of special case code seemed outside the problem domain. If this is sufficient to solve #121, please close that, otherwise please look at DateParser.parse_date. There may be a way to generalize this while keeping sensible defaults -- perhaps an optional block argument to set the format? Pull requests welcome!

cognitiveflux commented 9 years ago

This does ignore #121, and one could in theory set a :date column via over-riding the setter:

def created_at=(date_string)
   ....
   ns_date = ... # Parse the string and return an NSDate object
   write_attribute(:created_at, ns_date)
end

That being said, I created #121 because it seemed that they were two separate features. I'll continue the discussion within the #121 conversation.