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

Update result.rb #41

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

Update result.rb #41

wants to merge 2 commits into from

Conversation

s-meinhardt
Copy link

@s-meinhardt s-meinhardt commented Aug 13, 2022

What

Pass experiment.comparator no longer as a block via &experiment.comparator but as a proc to the comparator argument of the Observation.equivalent_to? method.

Why

This PR of the scientist gem from Oct 2021 changed the interface of the equivalent_to? method which no longer transforms an optional block into the comparator proc. Instead, one has to pass the comparator directly as a proc. Without that change, the comparator variable of equivalent_to? keeps its default value nil which leads to incorrect comparisons, e.g. Result.equivalent = false even though experiment.comparator returns true.

Note

We should also increase the minimal required scientist version in the Gemfile.lock as the interface change is not backward compatible. According to this commit we need at least 1.6.1 but it's 1.6.0 now.

[This PR](github/scientist#77) of the scientist gem from Oct 2021 changed the interface of the `equivalent_to?` method which no longer transforms an optional block into the `comparator` proc. Instead, one has to pass the comparator directly as a proc. Without that change, the `comparator` variable of `equivalent_to` keeps its default value `nil` which leads to incorrect comparisons, e.g. `Result.equivalent = false` even though `experiment.comparator` returns `true`.
@geeksam
Copy link
Contributor

geeksam commented Aug 9, 2023

Sorry, I left Real Geeks and no longer have commit privileges to this repo. Pinging @larslevie @kevin1024 @LupineDev @Sparkmasterflex

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.

2 participants