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

Allow setting raise_on_mismatches per Experiment instance #23

Merged
merged 5 commits into from
Dec 16, 2015

Conversation

tyamagu2
Copy link
Contributor

This patch addresses #21. I also find this would be useful, so here's the patch.

@tyamagu2 tyamagu2 force-pushed the raise_if_mismatches_per_instance branch from 43d583b to eb94336 Compare November 24, 2015 15:52
@@ -269,4 +269,16 @@ def try(name = nil, &block)
def use(&block)
try "control", &block
end

def raise_on_mismatches=(bool)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I might go with attr_accessor here instead, but continue to define the predicate method below.

@@ -5,6 +5,9 @@
# implements Scientist::Experiment's interface.
module Scientist::Experiment

# Whether to raise when the control and candidate mismatch.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you document the default behavior?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I did. Let me know if this is not what you wanted.

@tyamagu2
Copy link
Contributor Author

I'll rebase and squash if necessary, so let me know.

@jbarnette
Copy link
Contributor

@jesseplusplus @rick @zerowidth We let this one go stale. Final thoughts?

@rick
Copy link
Contributor

rick commented Dec 11, 2015

I'm neither a rebaser nor a squasher. Seems 👍 to 🚢.

jbarnette added a commit that referenced this pull request Dec 16, 2015
Allow setting raise_on_mismatches per Experiment instance
@jbarnette jbarnette merged commit 5fea1c6 into github:master Dec 16, 2015
@jbarnette
Copy link
Contributor

@jesseplusplus @zerowidth @rick The API seems pretty stable, this is the first change we've merged in a while. How bout bumping to 1.0.0?

@jesseplusplus
Copy link

@jbarnette Hmm, the master build failed CI after merging this. Looks like an issue installing the scientist gem while bundling before it even gets to the tests -- possibly related to a change in Travis' build environment for the older rubies since the last commit before merge passed with no troubles.

@jbarnette
Copy link
Contributor

@jesseplusplus Doh! I'll fix it up.

@jbarnette jbarnette mentioned this pull request Dec 17, 2015
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.

None yet

4 participants