Make sure raise error if you try to overwrite field name that is already defined in Object

[harada4atsushi]

From the request of #184 , an error is generated when you try to overwrite the method already defined in Object.

Is it better to use warnings instead of errors because the impact is great?

[syguer]

I don't know why we cannot use these names as attributes...(even if just a warning).
How does ActiveRecord behave?

[harada4atsushi]

@syguer
Thank you for reply.

I haven't read the source code enough yet, so I don't know the details, but if I define it with same name as a method of the Object class, It seems the Object class to be executed at runtime.

I checked the behavior of ActiveRecord.

create_table :countries do |t|
  t.string :display

  t.timestamps
end

class Country < ApplicationRecord
end

country = Country.new(display: 'hoge')
country.display

# => "hoge"

ActiveRecord seems to be overwritten with the defined field name.
I think it's appropriate to behave in the same way, so I'll review in detail when I have time.

[harada4atsushi]

I have found the relevant part, so write it down for the time being.

https://github.com/zilkey/active_hash/blob/2d16379b5865950ad90263294eb07b15d456a478/lib/active_hash/base.rb#L282-L288

If I remove the unless instance_methods.include?(field.to_sym) part, it will befave as exptected, but the behavior that pre-defined instance methods can be forcibly overwritten seems a little dangerous.

I tried to read what implementation of ActiveRecord is. I thought it was around the following, but I feel a little different.

https://github.com/rails/rails/blob/974ce9c3339441617da4c58a056087034fa25fc0/activemodel/lib/active_model/attribute_methods.rb#L284

[syguer]

@harada4atsushi
Thank you for the investigation. It seems a good way that ActiveHash behaves the same as ActiveRecord.
I think method overridden looks not so dangerous. Ruby allows method override anywhere and if the method is used inside system library, Ruby names it with special characters (e.g. __send__() ).
Anyway, I wonder why ActiveHash checks instance methods? We should look that history.

[harada4atsushi]

@syguer
I looked the change history of the relevant part.

f5e9e95

Apparently, it seems to be a change to avoid overriding the methods implemented in the class when defining fields using ActiveYaml.

This is not limited ActiveYaml, methods implemented in classes that inherit from ActiveHash::Base should not be overwritten by .field. For that purpose, It checked with instance_methods.include?(Field.to_sym), but in this case the parent class was also included, the method defined in Object class was not overwritten.

Therefore, it has been changed to instance_methods(false).include?(Field.to_sym) to allow overwriting methods other than those defined in the own class.

https://github.com/zilkey/active_hash/blob/50121efbc39b6f2c7b1acaf41d2c00ea68e4cf98/lib/active_hash/base.rb#L282-L288

[kbrock]

sorry, this is old but simple. Not sure why it stalled.

ActiveRecord does have a warning on dangerous field names.
Which means they kinda allow you to do anything.
Seems reasonable for us to do the same.

I like this solution and it looks pretty good.
Do wonder about the methods(false) when dealing with hierarchical classes (think ApplicationRecord) - but maybe that is over engineering on my side. Maybe these classes by definition will be smaller and simpler.

Any thoughts?

[kbrock]

Modifying a class in a before block often introduces sporadic test failures. Not sure if these test failures are passing, or sporadically passing.

Still concerned about hierarchical classes and the methods(false).