Feature #6056

Enhancements to OpenStruct

Added by Thomas Sawyer over 3 years ago. Updated almost 3 years ago.

[ruby-core:42779]
Status:Closed
Priority:Normal
Assignee:Marc-Andre Lafortune

Description

This patch fixes one issue, protecting #new_ostruct_member method, and possibly another by dup'ing marshal_dump, but I need more feedback on the later b/c I've also been told it is not needed.

The rest of this patch provides enhancements to OpenStruct that I feel are sorely needed, these include access via [] and []=, ability to mass update via merge!, minimal polymorphism with Hash and the addition of equality methods, eql? and ==.

https://github.com/ruby/ruby/pull/95

I'd also like opinions on further enhancements:

  • adding #each and #each_pair
  • making OpenStruct a subclass of BasicObject

Related issues

Related to Ruby trunk - Feature #1400: Please add a method to enumerate fields in OpenStruct Closed 04/23/2009
Related to Ruby trunk - Bug #6029: Should OpenStruct implement #eql? and #hash? Closed 02/15/2012

Associated revisions

Revision 37371
Added by Marc-Andre Lafortune almost 3 years ago

  • lib/ostruct.rb: Protect new_ostruct_member [#6056]

Revision 37371
Added by Marc-Andre Lafortune almost 3 years ago

  • lib/ostruct.rb: Protect new_ostruct_member [#6056]

Revision 37376
Added by Marc-Andre Lafortune almost 3 years ago

  • lib/ostruct.rb: Add [] and []=, base on a patch by Thomas Sawyer [Feature #6056]

Revision 37376
Added by Marc-Andre Lafortune almost 3 years ago

  • lib/ostruct.rb: Add [] and []=, base on a patch by Thomas Sawyer [Feature #6056]

History

#1 Updated by Marc-Andre Lafortune over 3 years ago

  • Assignee set to Marc-Andre Lafortune
  • Target version set to 2.0.0

Hi.

In the future, please break requests in independent parts.

  • new_ostruct_member will be protected

  • == is already defined. I don't think any coercion can be accepted in this case. You can't compare a Set and an Array directly, for instance. Open a new request if you disagree and show use case and how that would be consistent with other builtin and library classes.

  • eql? has already an issue opened: https://bugs.ruby-lang.org/issues/6029 . Please comment there if need be.

  • each_pair and to_h is the subject of https://bugs.ruby-lang.org/issues/1400 . Please comment there if need be.

  • marshal_dump is not meant for public use, no dup needed. I will remove the explicit documentation once issue 1400 is closed, as one should probably not rely on its implementation.

  • "making OpenStruct a subclass of BasicObject". I don't see why and can see different problem with it. Please make a separate request for it with justification (e.g. why should it would be more helpful if it did not implement respond_to?, send, object_id, etc...) if you want this.

This leaves "Access via [] and []=", which we can discuss in this thread. I was wondering about this myself, about why it wasn't implemented like this to start with. I'm inclined to think it would be a good idea.

#2 Updated by Thomas Sawyer over 3 years ago

Each change should be separate pull request or just a separate commit?

I knew about issue #6029, I was just trying to take care of that a couple of other features while I was involved with the code.

I see what you are saying about ==. I am actually surprised that:

class Q
def to_ary; [1,2,3]; end
end
Q.new == [1,2,3] #=> false

But that being the case, then okay I will remove.

I will also remove .dup from marshal_dump.

I will submit new issue about BasicObject, but briefly, the reason is for of use with #method_missing, to get as many methods out of the way as possible for it's use.

#3 Updated by Marc-Andre Lafortune over 3 years ago

Hi,

Thomas Sawyer wrote:

Each change should be separate pull request or just a separate commit?

Ideally, both. It makes it easier to accept only some of the parts. As I said, "in the future"; no need to split your existing commits. In this case, the difficulty lies in deciding what should be done, not in implementing it.

#4 Updated by Thomas Sawyer over 3 years ago

I've updated the pull request.

#5 Updated by Shyouhei Urabe over 3 years ago

  • Status changed from Open to Assigned

#6 Updated by Koichi Sasada almost 3 years ago

ping.
status?

#7 Updated by Marc-Andre Lafortune almost 3 years ago

  • Status changed from Assigned to Closed
  • % Done changed from 0 to 100

This issue was solved with changeset r37376.
Thomas, thank you for reporting this issue.
Your contribution to Ruby is greatly appreciated.
May Ruby be with you.


  • lib/ostruct.rb: Add [] and []=, base on a patch by Thomas Sawyer [Feature #6056]

Also available in: Atom PDF