krisleech / wisper

A micro library providing Ruby objects with Publish-Subscribe capabilities
3.27k stars 151 forks source link

Minimize calling to_s on class ancestors #179

Closed Looooong closed 1 year ago

Looooong commented 4 years ago

This PR is a follow-up of #174 and it solves issue with anonymous class string. It doesn't avoid calling to_s completely, but only calls to_s when allowed_classes contains a string like #<Class:0xXXXXXX>.

coveralls commented 4 years ago

Coverage Status

Coverage increased (+0.008%) to 99.886% when pulling 4ccc03dae4ada1eb3086d6af766073a529f85ade on Looooong:master into b29661a46b95801cb733918d11e8f8182a535697 on krisleech:master.

krisleech commented 4 years ago

Thanks for the input, is this branched off #174 or an alternative?

Looooong commented 4 years ago

This is an alternative.

Looooong commented 4 years ago

One more thing, I did copy the test suite from #174.

I also thought of another way to solve the issue with the anonymous class string "#<Class:0x005559d7de5338>". This involes using regex to get the object address (0x005559d7de5338) from this string and converting this address to object ID, which will then be fed to ObjectSpace._id2ref to get the original anonymous class. Here is an example:

klass = Class.new
klass.to_s # "#<Class:0x005559d7de5338>"
address = klass.to_s.match(/#<Class:0x([0-9a-f]+)>/).captures[0] # 005559d7de5338
ref = ObjectSpace._id2ref(address.to_i(16) >> 1)
ref == klass # true

With this solution, we can avoid calling to_s completely. What do you think?

krisleech commented 4 years ago

With this solution, we can avoid calling to_s completely. What do you think?

It's a bit fancy for my tastes (:smile:) and I wonder if it works in JRuby.

I'm not actually convinced we need to support anonymous classes. We do use anonymous classes in the specs but not because we want to support them :thinking:

@lkang @wheeyls any thoughts?

Looooong commented 4 years ago

It's a bit fancy for my tastes () and I wonder if it works in JRuby.

I don't work with JRuby so I have no idea.

On another note, ObjectSpace._id2ref is not safe that it can get any object exists in the memory. Special care should be taken to make sure that we get back a class or a module.

    def string_to_anonymous(klass)
      match = klass.match(/#<(Class|Module):0x([0-9a-f]+)>/)

      return if match.nil?

      type = match.captures[0]
      address = match.captures[1]
      ref = ObjectSpace._id2ref(address.to_i(16) >> 1)
      ref.class.name == type ? ref : nil
    end
Looooong commented 4 years ago

I have tested ObjectSpace._id2ref alternative and it indeed doesn't work with JRuby, and, concerningly, Ruby HEAD. In this case, I think we have the following options:

If we only use anonymous classes in the specs then we can replace anonymous class with a constant one.

wheeyls commented 4 years ago

Who can make the decision on whether to support anonymous classes? I don't feel like it would be my place to take a test out of the suite.

Looooong commented 4 years ago

This PR is a follow-up of #174 and it solves issue with anonymous class string. It doesn't avoid calling to_s completely, but only calls to_s when allowed_classes contains a string like #<Class:0xXXXXXX>.

@krisleech I decided to revert to my original implementation, simplify my code and add a couple of tests. It should be good now. I'm waiting for your review.

krisleech commented 4 years ago

https://github.com/RailsEventStore/rails_event_store/blob/v1.0.0/aggregate_root/lib/aggregate_root.rb#L17

In RailsEventStore they only allow anonymous classes with a self.name method, e.g.

Class.new do
  def name
   "MyClass"
  end
end
Looooong commented 4 years ago

Great! I will take a look at it.