-
Notifications
You must be signed in to change notification settings - Fork 239
Adding Lex v2 event and response #321
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
Conversation
...ambda-java-events/src/main/java/com/amazonaws/services/lambda/runtime/events/LexV2Event.java
Outdated
Show resolved
Hide resolved
Could you add a test into the serialization module? |
Thanks for the quick feedback, I have added the book trip example event as test case. However, for now there is a mismatch. The Json contains "slots": {
"ReturnDate": null,
"PickUpDate": null,
"DriverAge": null,
"CarType": null,
"PickUpCity": null
} while the result is "slots": {}, I'm unsure if that's the primary reason for the failing test and how to best fix this. Additionally, As the test Json right now does not contain all possible fields, I will see if I can capture them in the next few days. Finally, after having merge the current master the LambdaEventSerializersTests appear to have been skipped, at least on my end. |
private double mixed; | ||
private double negative; | ||
private double neutral; | ||
private double positiv; |
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.
is this a typo?
…nt are compiled for the correct architectures
…ng (aws#334) * Fix os compatibility tests by enabling multi-platform build and testing * Extract environment setup script
* Fix os compatibility test local builds on arm64 hosts * Extract log fetching and clean up to separate scripts
Could you check the tests? |
Hi @msailes, |
The
I don't know if this is bad data or possible? @Sordie do you know? |
Same for the |
@msailes thanks for pointing out where the configuration to exclude null values is, as I had not seen it before. |
@Sordie thank you. I think the tests you've added should be slightly changed then to show this behavior. The actual will be different to the json from the test file because maps which include items will null values are excluded. @andclt, @smirnoal I'm not sure how you want this implementing, maybe two files would be a suitable way? @andclt, @smirnoal I found this a hard problem to track down since there was no specific JSON knowledge in the testing lib. I would suggest with move to JSONassert either in this PR or another seperate one. This library gave much better error messages and I was able to spot the problem straight away. |
First of all thanks for contributing @Sordie! I agree with @msailes' suggestion. However, if you want to keep those null values after the serialization, I think you can achieve it adding the |
...ambda-java-events/src/main/java/com/amazonaws/services/lambda/runtime/events/LexV2Event.java
Outdated
Show resolved
Hide resolved
...ambda-java-events/src/main/java/com/amazonaws/services/lambda/runtime/events/LexV2Event.java
Outdated
Show resolved
Hide resolved
Hi @Sordie, As I said in a previous message on this thread, I think we should use the I was able to test successfully the following implementation, could you please take a look at it?
|
Hi @andclt, thanks for providing a code snippet. I will add your suggestion next week once I have a computer in front of me again. |
Hi @andclt, I have applied your suggestion and removed the now unused json files. |
Hi @Sordie, thank you! Approved! |
Issue #, if available:
#242
Description of changes:
Adding both Lex version 2 event and response as defined here: https://docs.aws.amazon.com/lexv2/latest/dg/lambda.html
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.