refactor: Use matcher and generator classes

[tienvx]

Depends on #373, please review it first

[JP-Ellis]

This is quite a big PR. I'll make time to review, but I may not get to it until mid next week if that's alright.

[tienvx]

It's totally fine. Thanks

[tienvx]

Hi @Lewiscowles1986 , @Greg0

Are you still interested in reviewing PRs from this project?

[Lewiscowles1986]

@tienvx

1. I don't think my reviews really matter;
2. I'm not sure about how this relates to FFI
3. While there are some lines I'd 👍 (broken time format) it's, generally a bit haphazard, and as mentioned above, it's a huge PR. As always I suggest any PR above 500 lines be treated with increasing caution.

[tienvx]

1. They are. I did agree with you in some suggestions in previous PRs. Please tell me where I miss. I will try to update if I can.
2. Yes, it's not related to FFI. It's just refactoring the code from a single big class to smaller classes.
3. I will try to break this PR into 3 smaller PRs.

[tienvx]

First PR: #375

~ 600 lines. Is it okay, or I need to split PRs for each generator?

[tienvx]

Nevermind, 600 lines PR is still too big, split again.

First PR: #376

[Greg0]

I will gladly look at this to the end of the year. Some covid here 😉

[Lewiscowles1986]

@tienvx if this were rebased, how many lines would now be delivered?

[tienvx]

It's about ~1400 lines

Which do you prefer? rebase or create new PR?

[tienvx]

Rebased for now. I will try to split into smaller PRs

[Lewiscowles1986]

No need to split, anything under 1000 lines (this was 2.5k) can be worked on. I Have a strong preference for soft-limit of 500 lines (ignoring lock files)

[Lewiscowles1986]

This one feels off. Do you have a bit more of an explanation on why the id has changed if the matcher is working?

[tienvx]

There are 2 methods:

• integer(13) used Type matching rule, with require an integer as example value. This matching rule doesn't support a generator.
• integerV3(13) used Integer matching rule. This one doesn't require example value. So it support generator.

In this example, I want to test ProviderState generator.

• Because Type doesn't support generator, I will got the (new) MatcherNotSupportedException exception.
• Integer, on the other hand, support generator, so we didn't get this exception.

So that's why I replace integer(13) by integerV3(13)

But there is 1 problem:

If I use integerV3(13), the generator ProviderState is still not working (it's because of PHP, not Rust. See GeneratorAwareMatcher)

That's why I replace integerV3(13) by just integerV3().

That's why 13 is replaced by null

[tienvx]

You may ask: Why on the old code, integer(13) still work with the generator ProviderState?

There are 2 reasons:

• I didn't do any checking in the code, just assign the pact:generator:type = 'ProviderState' into the final json.
• ProviderState generator is a bit different from other generators:
  • Other generators work on consumer side, so if there is example value (e.g. 13), generator will not have any effects.
  • ProviderState generator work on provider side. The example value (e.g. 13) will be replaced by the value from the provider state anyway.

[tienvx]

We can update GeneratorAwareMatcher, add additional check if (null === $this->getValue() || $this->generator instanceof ProviderState) {, but I think it's unnecessary condition.

[Lewiscowles1986]

I actually think the additional if statement will make it so that regression and surprise cannot take place, and the API is easier to think about. Computational cost vs user brain cost; but I'll approve (you've been very patient).

[tienvx]

I actually think the additional if statement will make it so that regression and surprise cannot take place...

It's a good point actually.

regression

Unfortunately, adding a simple condition if (null === $this->getValue() || $this->generator instanceof ProviderState) { doesn't help

surprise

I think we shouldn't allow these code at the first place:

• $this->matcher->fromProviderState($this->matcher->integer(123), '${id}')
• $this->matcher->fromProviderState($this->matcher->integerV3(123), '${id}')

Example value can't be used along with generator. Generator will be silently ignored without any notice (except for ProviderState, example value will be ignored). User will be surprised. So I created this PR to handle that #420

[Lewiscowles1986]

This is broadly very good. One question needs answering around the id changing; but once we have that down, this looks very good. Thank you for your patience with me.

[Lewiscowles1986]

You've done so much work here. Please be very proud!