-
Notifications
You must be signed in to change notification settings - Fork 137
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
Feature/add support and example messenger provider using rust engine 379 #700
Conversation
…iterator, other small tweaks
e10280f
to
43f37d6
Compare
…g-rust-engine-380' into feature/add-support-and-example-messenger-provider-using-rust-engine-379
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As with your other PR, excellent work!
I see there's a bit of duplication with your other PR, so we'll focus on merging one first before the other. I've left a few comments above.
There's a few minor errors in the typing (which I'm happy to fix).
Looking forward to merging the two PRs soon though!
[ | ||
pact.v3.ffi.message_with_metadata_v2(self._handle, k, v) | ||
for k, v in metadata.items() | ||
] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
While using list comprehension does achieve the desired outcome, a more regular for
loop would make the intent more obvious here :)
) -> AsyncMessagePactResult: | ||
reified_msg = self.reify() | ||
if not reified_msg: | ||
return [] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A couple of points here:
- The return type is incompatible with the annotation; but more importantly,
- Should this raise an error instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are two tests that fail if this raises an error:
- Scenario: Supports data for provider states
- Supports specifying provider states
In both cases, provider states are configured, but nothing else is. It seems to me like the verification should still continue in that scenario. I'll switch this up to continue with a default
] | ||
return self | ||
|
||
def reify(self) -> str: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we expect reify
to be ever used by end-users?
It would appear that reify
produces an output which isn't very user-friendly (as seen by the processing you have to do within verify
). So I would suggest that either:
reify
is modified to produce a more user-friendly output; provided that the function has a good use case for end users; or,- Remove
reify
or make it private.
else: | ||
msg = "Unexpected body definition" | ||
raise RuntimeError(msg) | ||
self._add_body(self.body, interaction) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice DRYing of the code!
…g-rust-engine-380' into feature/add-support-and-example-messenger-provider-using-rust-engine-379
closing in lieu of #725 |
📝 Summary
Feature/add support and example messenger consumer using rust engine
🔥 Motivation
Work for Add support and example for verifying a message provider using the Rust engine #379
🔨 Test Plan
Implemented tests for the compatibility suite. Specifically message provider tests for v3 and v4.