✨ Don't set default experiment for classes when `include`ing `Scientist::Experiment` by danielvdao · Pull Request #163 · github/scientist · GitHub
Skip to content

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

Open
danielvdao wants to merge 2 commits into
github:mainfrom
danielvdao:master
Open

danielvdao wants to merge 2 commits into
github:mainfrom
danielvdao:master

Conversation

@danielvdao

Copy link
Copy Markdown

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
Copy Markdown

@danielvdao

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

ekroon commented Apr 24, 2024

Copy link
Copy Markdown
Member

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