-
Notifications
You must be signed in to change notification settings - Fork 450
Support error comparison #77
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
Conversation
Lol, :'( I see the coveralls comment... and I can't seem to find the way to see where the coverage failed... although seems likely that the setup method |
62c3fb9
to
1f9a487
Compare
1 similar comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry I missed this earlier. This is a good improvement!
The only thing I'd suggest changing is the public API method name, from error_compare
to compare_errors
.
Could you add a README update describing the new option?
lib/scientist/experiment.rb
Outdated
# and return true or false. | ||
# | ||
# Returns the block. | ||
def error_compare(*args, &block) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think compare_errors
would be a better name for the method, at least it reads better to me:
science do |e|
e.compare { |a, b| ... }
e.compare_errors { |a, b| ... }
end
The instance variable doesn't need to change, though.
else | ||
a.equivalent_to? b | ||
end | ||
a.equivalent_to? b, @_scientist_comparator, @_scientist_error_comparator |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 handling nil arguments in the method is easier than this if
statement in the calling code.
lib/scientist/experiment.rb
Outdated
@@ -164,13 +174,9 @@ def name | |||
"experiment" | |||
end | |||
|
|||
# Internal: compare two observations, using the configured compare block if present. | |||
# Internal: compare two observations, using the configured compare and error_compare lambdas if present. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 to replacing "block", although you've replaced it here with "lambda" and elsewhere "proc"
@cdwort are you still planning on wrapping this one up? If not, I'm happy to jump in to help out. |
56b9479
to
c42ca0e
Compare
👋 @zerowidth @cdwort Hello from the future! 😸2️⃣0️⃣2️⃣1️⃣ I agree this would be a neat feature to have in Scientist, so I updated this PR based on the review comments from 2018. @cdwort I hope you do mind me pushing directly to your branch to make these updates 😅
Could I trouble you to re-review this PR and see if there's anything else we want to do? 🙏 Two notes:
Thanks! 🙇 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure this counts as a breaking change, as it only modifies an internal API. Either way though, I'm happy to bump major or minor for the next release.
Travis doesn't seem to be doing its thing anymore
Yeah, separate issue. GitHub Actions is a better alternative for CI builds here.
[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`.
`compare_errors` was introduced in PR github#77. Apparently, the latest renaming was forgotten to be applied to the README.
This is currently a breaking change!!
Fixes #50.
This PR changes the interface of
equivalent_to?
to support only two procs (compare
anderror_compare
). While this is the interface that I would write now, I imagine you'll want to keep backwards compatibility.What's your preferred means of doing that? Change the
equivalent_to?
method to be:If that looks good to y'all, I'll update this PR to use ^^ interface.
Thanks!!