github / scientist

:microscope: A Ruby library for carefully refactoring critical paths.
MIT License
7.44k stars 439 forks source link

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

Open danielvdao opened 3 years ago

danielvdao commented 3 years ago

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
bradenchime commented 3 years ago

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

danielvdao commented 1 year ago

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

ekroon commented 5 months ago

@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