Closed casperisfine closed 3 years ago
👍 I didn't look at this patch, but did you feel that there was a difficult that should be avoidable due to something odd in the new implementation?
d you feel that there was a difficult that should be avoidable due to something odd in the new implementation?
Well for what it's worth initialize_dup
calling back to initialize
did make it a bit complicated.
Other than that no, it's quite simpler.
Oh and totally unrelated, as it didn't break this gem, but the new precedence rule that allows the members to override public methods did cause us some grief as well (about 3 classes had to be fixed in our app). But TBH they shouldn't have used OpenStruct in the first place.
d you feel that there was a difficult that should be avoidable due to something odd in the new implementation?
Well for what it's worth
initialize_dup
calling back toinitialize
did make it a bit complicated.
You know, that's a good point. It's a bad decision on my part. Both initialize_dup
and initialize
should call a common method update!
or something. I'll tweak that.
but the new precedence rule that allows the members to override public methods did cause us some grief as well (about 3 classes had to be fixed in our app)
Interesting. Care to share more about this? I can't say that decision made me super happy in the first place.
But TBH they shouldn't have used OpenStruct in the first place.
Well, TBH, nobody should really be using OpenStruct
as it stands now 😆
Interesting. Care to share more about this?
Honestly they're pretty much all bugs on our side, typically:
class Foo < OpenStruct
def bar
42
end
end
p Foo.new(bar: 42).bar
We had a bit of code like this that depended on that precedence. Well depended isn't quite the right word as in all cases the foo:
key was actually useless and should never have been here in the first place.
So even though I don't believe it would break legitimate use cases, it's bound to break code in the wild. Either way we fixed it so not really a problem for us anymore.
The only semi legitimate one was a shitty mocha expectation:
OpenStruct.any_instance.expects(:foo).returns(:bar)
You can no longer stub a OpenStruct.new(foo: 42)
this way anymore since the method is redefine in initialize.
Well, TBH, nobody should really be using OpenStruct as it stands now
Agreed, I'm in the process of banning it from out code base, except maybe in the test/
directory.
Interesting. Care to share more about this?
Honestly they're pretty much all bugs on our side, typically:
class Foo < OpenStruct def bar 42 end end p Foo.new(bar: 42).bar
Right, so the conflicts were all with methods you were defining in a subclass of OpenStruct
? TBH, this is a good example where we it might be preferable to not allow overwriting it. Methods of Object
could be overriden by the hash passed to initialize
, but maybe not the subclasses' methods...
Thanks so much for these explanations.
The only semi legitimate one was a shitty mocha expectation:
OpenStruct.any_instance.expects(:foo).returns(:bar)
I'm not sure how any_instance
is implemented. The doc for the rspec
equivalent states "It is the most complicated feature of rspec-mocks, and has historically received the most bug reports. (None of the core team actively use it, which doesn't help.)".
Maybe the above restriction (overriding only Object's methods) might resolve this.
but maybe not the subclasses' methods...
I think that would be a fair behavior, that would prevent quite a bit of breakages, as I'm under the impression that quite often OpenStruct is used to wrap things such as API responses, and if you subclass it and define methods on it, it's fair to assume you don't want them replace.
I'm not sure how any_instance is implemented.
Not too sure either, but I wasn't planning to report it as a bug, I think it's fair that such mocks don't work so well with OpenStruct.
By the way @marcandre if you plan to do further changes to OpenStruct, feel free to ping me if you'd like me to run it against our test suite.
Since the gem already does a lot of feature testing/checking, I went ahead and added more.
Double-checking: are you referring to how the implementation figures out how to support inheriting from a given version of OpenStruct? (as opposed to adding more tests)
I'll leave this PR open for the moment for any additional feedback. Otherwise I'll aim to get it merged and released over the weekend.
FYI, I plan on making additional tweaks to OpenStruct
next week which will probably impact this PR. Doesn't mean this couldn't be merged and another PR opened later though.
are you referring to how the implementation figures out how to support inheriting from a given version of OpenStruct?
Yes, I'm talking about the various methods.include?
I plan on making additional tweaks to OpenStruct next week which will probably impact this PR. Doesn't mean this couldn't be merged and another PR opened later though.
There's no rush, if you can ping us back here when you think you're done with OpenStruct, I'll happily update the PR. I don't mind running a git branch for a couple weeks.
I've opened ruby/ruby#3593. This should make issues with initialize
easier to deal with
I would very much appreciate comments on ruby/ruby#3594 too.
I didn't check in depth your implementation but I'm surprised that you are not calling super
in []=
, or that you seem to be dupping again @table
in initialize_copy
. In particular, it's unclear to me as to why you are not overriding []=
to modify only it's argument (and call super
), which should be compatible with all versions. But as I said, I'm probably missing something and if that's the case, no need for lengthy explanations, I'll take your word for it 😆 Note: You(r) = nobody in particular
Just merged in master
and released as 0.3.0
. Let me know if you encounter any particular difficulty.
Sorry I was quite busy lately. I'm looking at this now, I'll try to simplify this as much as possible.
I didn't check in depth your implementation but I'm surprised that you are not calling super in []=
So I just tried that, but OpenStruct#[]=
and #[]
always cast they key as symbol. This gem seem to sometimes store both keys and strings :/
Ok, I simplified initialize_dup
to stop calling back to initialize
.
@aetherknight I believe this should be good to go.
A few changes were made to
OpenStruct
by @marcandre the last few days, two impacted this gem, and broke our ruby-master CI (NB we have a dependency on this gem throughkubeclient
):Since the gem already does a lot of feature testing/checking, I went ahead and added more.