Skip to content

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

Merged
merged 6 commits into from
Oct 22, 2021
Merged

Conversation

cdwort
Copy link
Contributor

@cdwort cdwort commented Dec 8, 2017

This is currently a breaking change!!

Fixes #50.

This PR changes the interface of equivalent_to? to support only two procs (compare and error_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:

def equivalent_to(other, error_comparator=nil, &comparator)

If that looks good to y'all, I'll update this PR to use ^^ interface.

Thanks!!

@coveralls
Copy link

coveralls commented Dec 8, 2017

Coverage Status

Coverage decreased (-0.5%) to 98.598% when pulling 62c3fb9 on cdwort:support_error_comparison into 25be7a2 on github:master.

@cdwort
Copy link
Contributor Author

cdwort commented Dec 8, 2017

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 error_compare is what I'm lacking.

@cdwort cdwort force-pushed the support_error_comparison branch from 62c3fb9 to 1f9a487 Compare December 8, 2017 20:12
@coveralls
Copy link

Coverage Status

Coverage increased (+0.009%) to 99.065% when pulling 1f9a487 on cdwort:support_error_comparison into 25be7a2 on github:master.

1 similar comment
@coveralls
Copy link

coveralls commented Dec 8, 2017

Coverage Status

Coverage increased (+0.009%) to 99.065% when pulling 1f9a487 on cdwort:support_error_comparison into 25be7a2 on github:master.

Copy link
Member

@zerowidth zerowidth left a 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?

# and return true or false.
#
# Returns the block.
def error_compare(*args, &block)
Copy link
Member

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

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.

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

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"

@joeltaylor
Copy link

@cdwort are you still planning on wrapping this one up? If not, I'm happy to jump in to help out.

@izuzak izuzak force-pushed the support_error_comparison branch from 56b9479 to c42ca0e Compare October 18, 2021 14:09
@izuzak
Copy link
Member

izuzak commented Oct 18, 2021

👋 @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 😅

  • 2a80f9c changes the naming from error_compare to compare_errors
  • 1bcbab1 fixes the failing tests
  • c42ca0e updates the README with instructions on the using compare_errors

Could I trouble you to re-review this PR and see if there's anything else we want to do? 🙏

Two notes:

  • *Travis doesn't seem to be doing its thing anymore -- no checks were created on the commits I pushed, and I see no checks were created on the PR that was most recently merged in this repository. I'm guessing something broke in the meantime, but I don't have admin access to this repo so that I can investigate. The tests are running fine locally ✅, but if we want to fix Travis builds for the website, someone with admin access would need to do that. I'd also be happy to take a look if you'd be brave enough to give me admin access 😬
  • As @cdwort mentioned, this is breaking change due to the change in the interface. With that in mind, the new version we release after merging this would need to bump the major version number. Personally, I like the new interface and I don't think releasing a new major version would be a problem. But I'd also be okay with considering other approaches if you don't feel like making a breaking change here. ✌️

Thanks! 🙇

@izuzak izuzak requested a review from zerowidth October 18, 2021 14:32
@izuzak izuzak self-assigned this Oct 18, 2021
Copy link
Member

@zerowidth zerowidth left a 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.

Co-authored-by: Nathan Witmer <nathan@zerowidth.com>
@izuzak izuzak merged commit 3fa27b8 into github:master Oct 22, 2021
s-meinhardt added a commit to s-meinhardt/lab_tech that referenced this pull request Aug 13, 2022
[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`.
franzliedke added a commit to franzliedke/scientist that referenced this pull request Sep 23, 2022
`compare_errors` was introduced in PR github#77. Apparently, the latest renaming was forgotten to be applied to the README.
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.

No way to compare exceptions
6 participants