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: Add url matcher #421

Merged
merged 1 commit into from
Dec 19, 2023
Merged

feat: Add url matcher #421

merged 1 commit into from
Dec 19, 2023

Conversation

tienvx
Copy link
Contributor

@tienvx tienvx commented Dec 19, 2023

This matcher is inspired by url() and url2() matchers from pact-js

This PR also confirm my claim from another PR.

// Temporary fix for inconsistancies between matchers and generators. Matchers use "value" attribute for
// example values, while generators use "example"

I think this is not inconsistent problem, but it's actually a genius idea from Pact core team. We can remove this line from pact-js and it still work fine

if (basePath == null) {
    return {
      // Remove this line
      // value: example,
    };
  }

@tienvx
Copy link
Contributor Author

tienvx commented Dec 19, 2023

cc @mefellows what do you think?

I know making the code consistent between pact-* project is important, but I can't help thinking that pact-js is making it a bit more complicated than it need to be.

@mefellows
Copy link
Member

I know making the code consistent between pact-* project is important, but I can't help thinking that pact-js is making it a bit more complicated than it need to be.

What problem are you referring to - having url and url2? I think url2 is the correct implementation, but because of compatibility reasons we couldn't change the implementation.

@tienvx
Copy link
Contributor Author

tienvx commented Dec 19, 2023

No, I am referring to the method signature url2(basePath: string | null, pathFragments: Array<string | V3RegexMatcher | RegExp>)

My approach has different method signature url(string $url, string $regex, bool $useMockServerBasePath = true), which is not consistent with pact-js.

The implement is still quite the same.

Is it okay for the inconsistent like this?

@mefellows
Copy link
Member

That might be a question for @uglyog. It looks to be semantically equivalent, perhaps it was a usability consideration?

@mefellows
Copy link
Member

Copy link
Collaborator

@Lewiscowles1986 Lewiscowles1986 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this looks really nice. Sounds like @mefellows had a similar understanding, that it is more important to be semantically equivalent than syntactically identical. Yet more nice work!

@tienvx tienvx merged commit c589f89 into pact-foundation:ffi Dec 19, 2023
26 checks passed
@tienvx tienvx deleted the add-url-matcher branch December 19, 2023 07:40
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