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

feat: Allow combining matchers #597

Merged
merged 1 commit into from
May 10, 2024

Conversation

tienvx
Copy link
Contributor

@tienvx tienvx commented May 3, 2024

Implemented this feature pact-foundation/pact-reference#405

@tienvx tienvx requested a review from JP-Ellis May 3, 2024 06:31
@tienvx tienvx force-pushed the combined-matchers branch from 5cb4035 to 623bb43 Compare May 3, 2024 06:37
@tienvx tienvx requested a review from mefellows May 3, 2024 06:49
@mefellows
Copy link
Member

Nice! Wow you got onto this feature quickly, I need to get my head into how a consumer might look, ad perhaps try with Pact JS/Go to test it out.

Would a review early next week work or is that going to be too late for you?

@tienvx
Copy link
Contributor Author

tienvx commented May 3, 2024

A review early next week would be great. I will focus on other things for now. Thanks

@tienvx tienvx mentioned this pull request May 5, 2024
@YOU54F
Copy link
Member

YOU54F commented May 10, 2024

This looks great.

With regard to

Pact JVM supports combining matching rules using the AND logical expression.

I was just looking to how this is implemented

https://github.com/pact-foundation/pact-jvm/blob/master/consumer/junit/README.md#combining-matching-rules-with-andor

It looks to support AND and OR operators.

and with regard to naming of MatchAll, if we were to have the ability to do an OR operation, what would that be called.

ahhh looking further, it doesn't look like OR is supported as part of pact-foundation/pact-reference#405

I just took a look at the matchers in pact-js and pact-go, to think about how the DSL would look. ie would be want to introduce a MatchAll function there. The pact-jvm DSL is clunkier for matcher bodies than other languages, so your approach might be preferable.

@tienvx
Copy link
Contributor Author

tienvx commented May 10, 2024

ahhh looking further, it doesn't look like OR is supported

Yes, it doesn't support. But I assume it will support in the future, and I suggest matchAny for it.

@tienvx
Copy link
Contributor Author

tienvx commented May 10, 2024

Self-reviewed

@tienvx tienvx merged commit 4b15877 into pact-foundation:master May 10, 2024
26 checks passed
@tienvx tienvx deleted the combined-matchers branch May 10, 2024 14:22
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.

3 participants