-
Notifications
You must be signed in to change notification settings - Fork 2.2k
fix(@aws-amplify/datastore): Nullable Arrays #6784
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
Fix array notation bug related to #5040
…o fix-nullable-arrays
|
This pull request introduces 1 alert when merging 9a763a7 into 6abbf50 - view on LGTM.com new alerts:
|
Codecov Report
@@ Coverage Diff @@
## main #6784 +/- ##
=======================================
Coverage 73.34% 73.34%
=======================================
Files 213 213
Lines 13159 13165 +6
Branches 2568 2571 +3
=======================================
+ Hits 9651 9656 +5
- Misses 3314 3315 +1
Partials 194 194
Continue to review full report at Codecov.
|
iartemiev
left a comment
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.
Looks great! Just a minor naming change requested
packages/datastore/src/types.ts
Outdated
| | EnumFieldType; | ||
| isArray: boolean; | ||
| isRequired?: boolean; | ||
| isArrayRequired?: boolean; |
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 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.
Hey @seang96, thank you so much for this PR! I talked it over with @yuth from the CLI team and we wanted to see if you could change
isArrayRequired->isArrayNullablein this PR, as well as in your related CLI PR.
We'll merge both PRs promptly once the change has been made.
I pushed the respective changes! Sorry for the delay, got caught up in other projects.
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.
No worries, thanks for updating! We should flip the behavior as well though since isNullable has effectively the opposite meaning of isRequired, i.e. where you had isArrayRequired should now be !isArrayNullable. Sorry for the confusion.
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.
No problem, I did not think about the implications for the name change haha. Commit is pushed.
…nto fix-nullable-arrays
Due to name change, value should be the opposite.
…nto fix-nullable-arrays
|
This pull request has been automatically locked since there hasn't been any recent activity after it was closed. Please open a new issue for related bugs. Looking for a help forum? We recommend joining the Amplify Community Discord server |
Issue #, if available: #6500
Description of changes:
This is based off of ondojo's initial PR tagged above.
I added a new field that gets stored in the schema generated from @aws-amplify/cli called isArrayRequired.
PR to update @aws-amplify/cli - aws-amplify/amplify-cli#5327
I added tests for the nullable arrays
This PR should support the table below:
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.