dry-rb / dry-core

A toolset of small support modules used throughout the @dry-rb & @rom-rb ecosystems
https://dry-rb.org/gems/dry-core/
MIT License
171 stars 34 forks source link

Add support for BasicObject #47

Closed oleander closed 3 years ago

oleander commented 3 years ago

As discussed in https://github.com/dry-rb/dry-core/pull/46

oleander commented 3 years ago

I don’t think that will work whenever the object is frozen.

fre 22 jan. 2021 kl. 05:35 skrev Piotr Solnica notifications@github.com:

@solnic commented on this pull request.

In lib/dry/core/memoizable.rb https://github.com/dry-rb/dry-core/pull/47#discussion_r562382627:

@@ -12,7 +12,14 @@ def memoize(*names)

     def new(*)
       obj = super
  • obj.instance_eval { @memoized = MEMOIZED_HASH.dup }
  • case obj
  • when Object
  • obj.instance_variable_set(:'@memoized', MEMOIZED_HASH.dup)
  • else
  • obj.instance_eval { @memoized = MEMOIZED_HASH.dup }
  • end

What Nikita said or maybe this should be set lazily via def memoized; @memoized ||= MEMOIZED_HASH.dup; end?

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/dry-rb/dry-core/pull/47#discussion_r562382627, or unsubscribe https://github.com/notifications/unsubscribe-auth/AABV5GYT67PKDPB5JQEA6H3S3D6BNANCNFSM4WLBXEZQ .

oleander commented 3 years ago

@solnic ~Any idea why the test suite fails? It passes locally and the errors seem unrelated to my changes.~ I did a git commit ---amend and a forced push (no changes) which solved the problem. Very odd :)

flash-gordon commented 3 years ago

I fixed specs in https://github.com/dry-rb/dry-core/commit/bcec0a6039d9a4096ca23a204705ad45cdd71586

oleander commented 3 years ago

@solnic Let me know if you need any benchmarks on this. The solution suggested by @flash-gordon has to be faster than my initial proposal, so I don't think any further benchmarking is needed–but don't quote me on that.

solnic commented 3 years ago

@oleander could you rebase/squash this so there's just one commit to merge? 🙇🏻

oleander commented 3 years ago

@solnic Squashed!