scientist icon indicating copy to clipboard operation
scientist copied to clipboard

✨ Don't set default experiment for classes when `include`ing `Scientist::Experiment`

Open danielvdao opened this issue 4 years ago • 3 comments

Relates to https://github.com/github/scientist/issues/162. I'm happy to close if it's unnecessary!

Thoughts: I found that making a class the default experiment a little aggressive and wondered if there was another way around this. It looks like in the code I could do something like this, but this seems not ideal for two reasons:

  1. I would want to do Scientist::Experiment.set_default(Scientist::Default) to preserve backwards compatibility for current classes that include Scientist, but not Scientist::Experiment
  2. Violates open-closed principle (e.g. if I wanted to do the above, then if it was decided that Scientist::Default was not the default class, then this could be a breaking change for gem users)

That said, I realized now that this might be my ignorance around the gem -- but it looks like running a scientist experiment class in isolation might not be "the right way to use the library" 😝 and I could've been wrong to begin with? Though I am wondering, if this allows for us to actually run multiple experiments at once 🤔 I attached a code snippet of what I was doing, and would love to hear thoughts!

I was doing something like the following:

class Foo
  include Scientist::Experiment
end

then:

      experiment = Foo.new.tap do |e|
        e.context(user_id: overdraft.user.id)
        e.try { limit_calculator.calculate_max_eligible_limit_new }
        e.use { limit_calculator.calculate_max_eligible_limit }
      end

      result = experiment.run

danielvdao avatar Sep 20 '21 06:09 danielvdao

Would be nice to update the README to reflect/provide guidance on how to use this change 👍

bradenchime avatar Sep 22 '21 21:09 bradenchime

Hey @zerowidth is there any feedback / thoughts you have about this? There seems to be a lot of comments from folks on this issue.

danielvdao avatar Feb 28 '23 17:02 danielvdao

@danielvdao I am not sure if what you are doing is the intent of the gem. I think that overriding the default class for the experiment is useful for publishing / etc, but there is no need to include Scientist::Experiment in more than one class in general.

For your example I would either include Scientist in the class running the experiment and do something like:

class SomeClass
  include Scientist
  
  def run_foo
    foo = Foo.new
    science "experiment" do |e|
      e.try { foo.xxxxx } 
      e.use { foo.yyyyy }
    end
  end
end

Or if that is not possible then run through Scientist.run:

class SomeClass

  def run_foo
    foo = Foo.new
    Scientist.run "experiment" do |e|
      e.try { foo.xxxxx }
      e.use { foo.yyyyy } 
    end
  end
end

ekroon avatar Apr 24 '24 10:04 ekroon