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

Added Add, Or, Not, WithTransform matchers for composability. #108

Merged
merged 7 commits into from Sep 27, 2015

Conversation

Projects
None yet
3 participants
@jim-slattery-rs
Contributor

jim-slattery-rs commented Sep 6, 2015

  • Allows matchers to be composed into complex expressions that work even with the Eventually() assertion.
  • This can make it easy to create new matchers. A single function could return a new matcher assembled from existing ones, rather than having to create a new class and implement the full types.GomegaMatcher interface from scratch.
Added Add, Or, Not, WithTransform matchers, for composability.
- Allows matchers to be composed into complex expressions that work even with the Eventually() assertion.
- Also makes it easy to create new matchers -- can often write a function that composes a new matcher out of existing ones.
@onsi

This comment has been minimized.

Show comment
Hide comment
@onsi

onsi Sep 7, 2015

Owner

Thanks for the PRs! Am going to take a look and give them some thought and will get back to you soonz

Owner

onsi commented Sep 7, 2015

Thanks for the PRs! Am going to take a look and give them some thought and will get back to you soonz

@onsi

This comment has been minimized.

Show comment
Hide comment
@onsi

onsi Sep 12, 2015

Owner

Hey @jim-slattery-rs this is good stuff - sorry for the delay, life has been busy!

I have a few conversation points and suggestions. Am happy to work with you on all of this, too.

  1. WithTransform is awesome. I'd suggest changing the signature to WithTransform(transform interface{}, matcher types.GomegaMatcher) and updating the docs to say (and code, to assert) that transform must be a function that takes one argument and returns one result. This would help avoid an unnecessary cast and give the developer a little more help from the compiler:
var plus1 = func(i interface{}) interface{} { return i.(int) + 1 }

would become

var plus1 = func(i int) int { return i + 1 }
  1. Adding boolean combinations makes a lot of sense but they break up the sentence structure. We could, potentially, keep And and Or in but also have aliases SatisfyAll and SatisfyAny. That gives a more natural:
Ω(foo).Should(SatisfyAll(ContainElement("bar"), HaveLen(3)))
Expect(foo).To(SatisfyAny(ContainElement("bar"), HaveLen(3)))
  1. Gomega has support for aborting an Eventually immediately. If a GomegaMatcher implements MatchMayChangeInTheFuture(actual interface{}) bool then Eventually will poll this method and abort early. I think we can and should appropriately combine and forward the MatchMayChangeInTheFuture return values of any matchers combined with all the methods you've added.

Details are in the docs. I'd be happy to help with this.

  1. Would love to update the docs at http://onsi.github.io/gomega to mention these matchers. Would also be nice to update the Custom Matchers section with examples of how a custom matcher can now just be a function that mixes together existing matchers with the methods you've added. Am happy to update the docs myself, but also happy to pull in a PR if you're up for it.
Owner

onsi commented Sep 12, 2015

Hey @jim-slattery-rs this is good stuff - sorry for the delay, life has been busy!

I have a few conversation points and suggestions. Am happy to work with you on all of this, too.

  1. WithTransform is awesome. I'd suggest changing the signature to WithTransform(transform interface{}, matcher types.GomegaMatcher) and updating the docs to say (and code, to assert) that transform must be a function that takes one argument and returns one result. This would help avoid an unnecessary cast and give the developer a little more help from the compiler:
var plus1 = func(i interface{}) interface{} { return i.(int) + 1 }

would become

var plus1 = func(i int) int { return i + 1 }
  1. Adding boolean combinations makes a lot of sense but they break up the sentence structure. We could, potentially, keep And and Or in but also have aliases SatisfyAll and SatisfyAny. That gives a more natural:
Ω(foo).Should(SatisfyAll(ContainElement("bar"), HaveLen(3)))
Expect(foo).To(SatisfyAny(ContainElement("bar"), HaveLen(3)))
  1. Gomega has support for aborting an Eventually immediately. If a GomegaMatcher implements MatchMayChangeInTheFuture(actual interface{}) bool then Eventually will poll this method and abort early. I think we can and should appropriately combine and forward the MatchMayChangeInTheFuture return values of any matchers combined with all the methods you've added.

Details are in the docs. I'd be happy to help with this.

  1. Would love to update the docs at http://onsi.github.io/gomega to mention these matchers. Would also be nice to update the Custom Matchers section with examples of how a custom matcher can now just be a function that mixes together existing matchers with the methods you've added. Am happy to update the docs myself, but also happy to pull in a PR if you're up for it.
@onsi

This comment has been minimized.

Show comment
Hide comment
@onsi

onsi Sep 19, 2015

This is potentially dangerous. I wonder what your thoughts are:

Typically folks don't save off matchers and this sort of approach works OK. However, I could see people creating and saving off and reusing WithTransform matchers. Relying on m.transformedValue could lead to data races and/or confusing error messages? Maybe?

The fix is to pull out a private helper method and always transform actual. I'm not convinced this is necessary so I wonder what you think.

This is potentially dangerous. I wonder what your thoughts are:

Typically folks don't save off matchers and this sort of approach works OK. However, I could see people creating and saving off and reusing WithTransform matchers. Relying on m.transformedValue could lead to data races and/or confusing error messages? Maybe?

The fix is to pull out a private helper method and always transform actual. I'm not convinced this is necessary so I wonder what you think.

This comment has been minimized.

Show comment
Hide comment
@jim-slattery-rs

jim-slattery-rs Sep 19, 2015

Owner

Very good point, I think that's a good idea to just always transform actual, in that case.

Very good point, I think that's a good idea to just always transform actual, in that case.

This comment has been minimized.

Show comment
Hide comment
@jim-slattery-rs

jim-slattery-rs Sep 19, 2015

Owner

Actually, I did the same thing in AndMatcher and OrMatcher -- they both needed to store some state from Match to use in the failure messages..
What do you think, is it worthwhile to perform the matches over again to avoid storing that state? I'm not sure if these matchers are reused in a multithreaded environment where it would become an issue?

Actually, I did the same thing in AndMatcher and OrMatcher -- they both needed to store some state from Match to use in the failure messages..
What do you think, is it worthwhile to perform the matches over again to avoid storing that state? I'm not sure if these matchers are reused in a multithreaded environment where it would become an issue?

This comment has been minimized.

Show comment
Hide comment
@onsi

onsi Sep 19, 2015

You know what, maybe it's a non-issue.

I bet most usages will look like:

HaveColor := func(c Color) GomegaMatcher {
    return WithTransform(func(e Element) Color {
        return e.Color
    }, Equal(c))
}

I.e. No reuse.

onsi replied Sep 19, 2015

You know what, maybe it's a non-issue.

I bet most usages will look like:

HaveColor := func(c Color) GomegaMatcher {
    return WithTransform(func(e Element) Color {
        return e.Color
    }, Equal(c))
}

I.e. No reuse.

This comment has been minimized.

Show comment
Hide comment

Nice :)

jim-slattery-rs added some commits Sep 19, 2015

make And() propagate MatchMayChangeInTheFuture()
- considers both Match() cases and considers the appropriate matcher(s) in making decision
- also added a comment to WithTransform.MatchMayChangeInTheFuture(), as it will not have correct behavior with non-deterministic transformer
@jim-slattery-rs

This comment has been minimized.

Show comment
Hide comment
@jim-slattery-rs

jim-slattery-rs Sep 20, 2015

Contributor

@onsi I think I've addressed all of your feedback!

  • WithTransform() is looking much improved, now able to use explicit types in the transform function
  • SatisfyAll() and SatisfyAny() are available as aliases for And() and Or()
  • I gave some thought to MatchMayChangeInTheFuture() and added support for it to all the matchers (Not(), And(), Or(), WithTransform())
    • Not() was simple
    • And() and Or() were a little trickier, but I think I've figured it out so they do the right thing in all cases
    • WithTransform() was simple, but I left a note about how it might not do the right thing when the transform function is non-deterministic. Either: 1) don't worry about such a rare edge case, 2) just always return true and sacrifice performance for correctness, or maybe some 3rd alternative, like adding WithNonDeterministicTransform() (yuck) to handle that edge case?

I think that's everything for this PR, thanks for your feedback so far. I haven't updated the docs website yet, I take it that will be a separate PR on the gh-pages branch?

Thanks,
Jim

Contributor

jim-slattery-rs commented Sep 20, 2015

@onsi I think I've addressed all of your feedback!

  • WithTransform() is looking much improved, now able to use explicit types in the transform function
  • SatisfyAll() and SatisfyAny() are available as aliases for And() and Or()
  • I gave some thought to MatchMayChangeInTheFuture() and added support for it to all the matchers (Not(), And(), Or(), WithTransform())
    • Not() was simple
    • And() and Or() were a little trickier, but I think I've figured it out so they do the right thing in all cases
    • WithTransform() was simple, but I left a note about how it might not do the right thing when the transform function is non-deterministic. Either: 1) don't worry about such a rare edge case, 2) just always return true and sacrifice performance for correctness, or maybe some 3rd alternative, like adding WithNonDeterministicTransform() (yuck) to handle that edge case?

I think that's everything for this PR, thanks for your feedback so far. I haven't updated the docs website yet, I take it that will be a separate PR on the gh-pages branch?

Thanks,
Jim

make Or() support MatchMayChangeInTheFuture()
- simplify tests by using empty And() or Or() instead of Receive() with a closed channel
@onsi

This comment has been minimized.

Show comment
Hide comment
@onsi

onsi Sep 27, 2015

Owner

@jim-slattery-rs this looks good -- will pull it in and might make some cosmetic changes.

A separate PR to the gh-pages branch would be great if you're up for it. Sorry for the long feedback loops - I'm finding I only have time to work on this over the weekends :/

Owner

onsi commented Sep 27, 2015

@jim-slattery-rs this looks good -- will pull it in and might make some cosmetic changes.

A separate PR to the gh-pages branch would be great if you're up for it. Sorry for the long feedback loops - I'm finding I only have time to work on this over the weekends :/

onsi added a commit that referenced this pull request Sep 27, 2015

Merge pull request #108 from jim-slattery-rs/composition
Added Add, Or, Not, WithTransform matchers for composability.

@onsi onsi merged commit 0652f5f into onsi:master Sep 27, 2015

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@modocache modocache referenced this pull request Nov 20, 2015

Closed

This or that #176

@jim-slattery-rs jim-slattery-rs referenced this pull request Nov 22, 2015

Closed

MatchOneOf #50

@m00sey

This comment has been minimized.

Show comment
Hide comment
@m00sey

m00sey Dec 16, 2015

Just stumbled over this, exactly what I needed and an awesome addition. Thanks for the effort @jim-slattery-rs

m00sey commented Dec 16, 2015

Just stumbled over this, exactly what I needed and an awesome addition. Thanks for the effort @jim-slattery-rs

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment