-
Notifications
You must be signed in to change notification settings - Fork 254
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/custom pk model identifier #1430
Feat/custom pk model identifier #1430
Conversation
bfee2ea
to
e6eb568
Compare
Codecov Report
@@ Coverage Diff @@
## feat/custom-pk #1430 +/- ##
==================================================
+ Coverage 46.17% 46.51% +0.33%
==================================================
Files 256 260 +4
Lines 10078 10181 +103
==================================================
+ Hits 4654 4736 +82
- Misses 5424 5445 +21
Flags with carried forward coverage won't be shown. Click here to find out 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.
I could be missing it - I don't see any models using a custom primary key
packages/amplify_api/lib/src/graphql/paginated_result_impl.dart
Outdated
Show resolved
Hide resolved
packages/amplify_datastore/example/lib/models/BelongsToChildExplicit.dart
Show resolved
Hide resolved
@@ -31,10 +31,10 @@ class SimpleCustomType { | |||
try { | |||
return _foo!; | |||
} catch (e) { | |||
throw DataStoreException( | |||
DataStoreExceptionMessages | |||
throw AmplifyCodeGenModelException( |
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.
This is a breaking change we should note
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.
Hum I missed this part somehow, I don't this this is an expected change, let me double check, could be just the test model is outdated (generated with WIP codegen of other features)
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.
I believe this is expected change in codegen that is specific to > 0.4.0, though possible we did not note this change properly. However, I think any breakage was already with 0.3.x - 0.4.x
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.
LGTM, spoke offline about an integ test failure and want to resolve that before approving (or if non-issue with implementation and just tests, can be followup PR IMO).
The integ test failure should be caused by the issue that we patched in 0.4.1. I will update the PR branch. |
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.
approved assuming integ test issue resolved w rebase
Thanks for the follow up @ragingsquirrel3 ! yes the integration tests all passed after merging the main branch. |
- Update DataStore unit tests to use test_models provided by amplify_test package - Update API unit tests to use updated test models
73c5c5b
to
e46de6a
Compare
* Convert Model.dart to LF mode * feat(datastore): Adding ModelIdentifier interface * - Update test models t conform interface changes - Update DataStore unit tests to use test_models provided by amplify_test package - Update API unit tests to use updated test models * Update DataStore example App models * Re-apply change after merge main * Regenerated test models with fix adding @OverRide * Fix transitive dependency error * Restore unexpected change * Add unit tests for modelIdentifier getter
* Convert Model.dart to LF mode * feat(datastore): Adding ModelIdentifier interface * - Update test models t conform interface changes - Update DataStore unit tests to use test_models provided by amplify_test package - Update API unit tests to use updated test models * Update DataStore example App models * Re-apply change after merge main * Regenerated test models with fix adding @OverRide * Fix transitive dependency error * Restore unexpected change * Add unit tests for modelIdentifier getter
Issue #, if available:
#1426
Description of changes:
Model.getId
observeQuery
in DataStore which usesgetId()
in a separate PRmodelIdentifier
getter toModel
amplify_test
packageBy submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.