adding matcher POC #761
adding matcher POC #761
Conversation
There was a problem hiding this comment.
This is just to serve the expected response and would need to be heavily modified for a final product.
src/pact/v3/matchers/matchers.py
Outdated
There was a problem hiding this comment.
I implemented a generator option only in the regex matcher again just as a POC.
There was a problem hiding this comment.
The Matcher class will (I think) be useful when implementing more complex logic, just as being able to "OR" together different matchers. As far as I can tell, there's no way to do the "OR" logic directly in parameter JSON (e.g. { "attribute": { "value": "expected", "pact:matcher:type": "regex", "regex": "expect.*" } }.
valkolovos
force-pushed
the
matcher_poc
branch
5 times, most recently
from
84dbcae
to
e5939c3
Compare
examples/tests/v3/test_matchers.py
Outdated
| from pathlib import Path | ||
|
|
||
| from examples.tests.v3.basic_flask_server import start_provider | ||
| from pact.v3 import Pact, Verifier, matchers |
There was a problem hiding this comment.
I would want to avoid polluting the namespace, so I agree with your choice of importing a module and calling matchers.like and matchers.regex etc.
My nit here is that I would want to name it match so that the syntax is match.regex(...) and match.like(...), etc. This way, it reads a bit more like English :)
There was a problem hiding this comment.
I feel like the module that defines the actual classes (Matcher, MatcherEncoder, etc) should be called "matchers" since they are objects. I'd be happy to create a module called match and move the functions that return those matchers into that module.
There was a problem hiding this comment.
Just started going through your updated PR. I'm still thinking that we should expose the the functions behind a common match.{func} interface, as I do think it becomes most intuitive. It also serves as a nice way to distinguish match.{func} from similar generate.{func}.
I also agree that having matchers as the module name for the Matcher classes makes sense. Having said that, I don't think people would often (or ever?) need to interact with the class directly. So I'm liking what you've done whereby the matchers are defined in a submodule, and functions re-exported at a higher level.
src/pact/v3/interaction/_base.py
Outdated
| if not isinstance(body, str): | ||
| body_str = json.dumps(body, cls=MatcherEncoder) |
There was a problem hiding this comment.
Nit: Avoid negative isinstance assertions.
If Python was strongly typed, this wouldn't matter; however, the type annotations are merely hint and at runtime, end users can in fact input whatever they want. So we need to handle cases of with_body(MyClassWithouSerialization(...)) (which your if statement would catch).
valkolovos
force-pushed
the
matcher_poc
branch
3 times, most recently
from
4a97348
to
a159b47
Compare
|
I've started working on the PR. Pushed a few changes (which are so far relatively minor). I do anticipate making some larger changes at a later date, but I just want to get a feel for it first :) |
The `dict`, `list` and `tuple` types require argument. Also, the use of the lowercase variants is not compatible with Python 3.8 (which will be dropped soon anyway... but still need to support it for a little while longer). Signed-off-by: JP-Ellis <[email protected]>
The `JSONEncoder` class uses `o` as the argument and does not enforce a positional argument. This means we need to support the possible use of `default(o=...)`. Signed-off-by: JP-Ellis <[email protected]>
The type annotation that goes alongside a `**kwargs` types the _values_. Therefore a `**kwargs: Foo` will result in `kwargs` being of type `dict[str, Foo]`. Signed-off-by: JP-Ellis <[email protected]>
While equivalent, Optional and Union quickly can become quite verbose and more difficult to parse. Compare ``` Optional[Union[str, bool]] ``` to ``` str | bool | None ``` Signed-off-by: JP-Ellis <[email protected]>
They are functionally equivalent, but with ABC being made to be more intuitive than the use of metaclass Signed-off-by: JP-Ellis <[email protected]>
Signed-off-by: JP-Ellis <[email protected]>
Signed-off-by: JP-Ellis <[email protected]>
In particular, splitting up the class, functions and types into separate modules. Signed-off-by: JP-Ellis <[email protected]>
valkolovos
force-pushed
the
matcher_poc
branch
from
c8baa3c
to
4cd67f5
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #761 +/- ##
======================================
Coverage 78% 79%
======================================
Files 23 28 +5
Lines 2673 2813 +140
======================================
+ Hits 2111 2243 +132
- Misses 562 570 +8
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
The `match.types` module only provides typing information; therefore, it makes sense to have a minimal definition in `types.py` and to create a much more complex `types.pyi` stub. The stub is not evaluated at runtime, but is parsed by type checkers. As an additional benefit for the stubs, there's no need to conditionally import modules which may or may not be present. Signed-off-by: JP-Ellis <[email protected]>
This is the TypeVar equivalent of the Matchable union type. Signed-off-by: JP-Ellis <[email protected]>
Some minor renaming. Instead of `ConcreteMatcher`, prefer `GenericMatcher` as it leaves room for other mathcers to be created (e.g. `BooleanMatcher`, `BaseModelmatcher`, etc.). Secondly, made the matcher compatible with both the Integration JSON format and Matching Rules format (and created two separate encoders to go with these). Signed-off-by: JP-Ellis <[email protected]>
There was a problem hiding this comment.
TIL about stub files :)
matcher POC
I just want to verify if this is headed in the right direction