Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

✨ Don't set default experiment for classes when includeing Scientist::Experiment #163

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

danielvdao
Copy link

Relates to #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
Copy link

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

@danielvdao
Copy link
Author

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
Copy link
Member

ekroon commented Apr 24, 2024

@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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants