Skip to content
New issue

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

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Equality and better metaprogramming for RecordRef. #167

Closed
wants to merge 5 commits into from

Conversation

jdfrens
Copy link
Contributor

@jdfrens jdfrens commented Mar 2, 2015

Improvements to NetSuite::Records::RecordRef.

Improved metaprogramming:

  • Implements #respond_to_missing? (to complement #method_missing).

Added equality:

  • Must have same type.
  • Must have equal non-nil internal ids.
  • Internal ids are converted to strings before they are compared.
  • All other fields and attributes are ignored.
  • #hash is overridden to hash on the string version of the internal id.

* Only the internal id is considered.
* Like ActiveRecord, a nil internal id is never equal to anything else.
* Override #hash appropriately.
* Compare and hash on string representations of internal ids.
@iloveitaly
Copy link
Member

My instinct here is to leave the equality operators working as is. This seams to break the principal of least surprise: ActiveRecord, and other DB abstractions I've worked with, don't operate like this; I'm hesitant to break convention with the gems and systems I've worked with in the past.

Any thoughts here @greggroth?

@iloveitaly
Copy link
Member

Implements #respond_to_missing? (to complement #method_missing).

Could you explain why you needed to implement this? My meta programming knowledge might be a bit lacking here.

Thanks for the PRs! Keep them coming!

@jdfrens
Copy link
Contributor Author

jdfrens commented Mar 20, 2015

Actually, the definition of == here matches up with the one for ActiveRecord (see: http://stackoverflow.com/questions/4738439/how-to-test-for-activerecord-object-equality).

I just now found a blog post which suggests a few changes to my definition (like calling super). I'd be happy to make this change.

@jdfrens
Copy link
Contributor Author

jdfrens commented Mar 20, 2015

If you don't define #respond_to_missing?, then #respond_to? is inconsistent with the state of the object and #method won't work at all.

# WITHOUT respond_to_missing?
[1] (pry) main: 0> rr = NetSuite::Records::RecordRef.new(foo: 5)
#<NetSuite::Records::RecordRef:0x007fca15b280b8 @internal_id=nil, @external_id=nil, @type=nil, @attributes={:foo=>5}>
[2] (pry) main: 0> rr.foo
5
[3] (pry) main: 0> rr.respond_to?(:foo)
false
[4] (pry) main: 0> rr.method(:foo)
NameError: undefined method `foo' for class `NetSuite::Records::RecordRef'
from (pry):4:in `method'

Honestly, I have no need for this right now, but I wanted equality for my specs. Since I was in this file anyway, I thought I'd add #respond_to_missing?.

other.class == self.class &&
other.internal_id != nil && self.internal_id != nil &&
other.internal_id.to_s == self.internal_id.to_s
end
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems reasonable to me

# Conflicts:
#	lib/netsuite/records/customer.rb
#	spec/netsuite/records/customer_spec.rb
@drewish
Copy link
Contributor

drewish commented May 21, 2015

Looks like there's a year old PR to add respond_to_missing?: #73

@jdfrens jdfrens closed this May 22, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants