Skip to content
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

Merged

Conversation

HuiSF
Copy link
Member

@HuiSF HuiSF commented Mar 4, 2022

Issue #, if available:

#1426

Description of changes:

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

@HuiSF HuiSF requested a review from a team as a code owner March 4, 2022 01:05
@HuiSF HuiSF force-pushed the feat/custom-pk-model-identifier branch from bfee2ea to e6eb568 Compare March 4, 2022 17:19
@codecov-commenter
Copy link

codecov-commenter commented Mar 4, 2022

Codecov Report

Merging #1430 (59d3879) into feat/custom-pk (8c74574) will increase coverage by 0.33%.
The diff coverage is 0.00%.

@@                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     
Flag Coverage Δ
android-unit-tests ∅ <ø> (∅)
flutter-unit-tests 36.66% <0.00%> (-0.01%) ⬇️
ios-unit-tests 85.12% <ø> (-0.25%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
...kages/amplify_core/lib/src/types/models/model.dart 0.00% <0.00%> (ø)
...ter/example/ios/unit_tests/AtomicResultTests.swift 91.83% <0.00%> (ø)
...e/ios/unit_tests/MockAnalyticsCategoryPlugin.swift 15.38% <0.00%> (ø)
.../ios/unit_tests/amplify_flutter_exampleTests.swift 94.59% <0.00%> (ø)
...plify_flutter/example/ios/Runner/AppDelegate.swift 0.00% <0.00%> (ø)

@ragingsquirrel3 ragingsquirrel3 self-requested a review March 10, 2022 17:51
Copy link
Contributor

@dnys1 dnys1 left a 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

@@ -31,10 +31,10 @@ class SimpleCustomType {
try {
return _foo!;
} catch (e) {
throw DataStoreException(
DataStoreExceptionMessages
throw AmplifyCodeGenModelException(
Copy link
Contributor

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

Copy link
Member Author

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)

Copy link
Contributor

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

Copy link
Contributor

@ragingsquirrel3 ragingsquirrel3 left a 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).

@HuiSF
Copy link
Member Author

HuiSF commented Mar 11, 2022

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.

Copy link
Contributor

@ragingsquirrel3 ragingsquirrel3 left a 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

@HuiSF
Copy link
Member Author

HuiSF commented Mar 21, 2022

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.

@HuiSF HuiSF force-pushed the feat/custom-pk-model-identifier branch from 73c5c5b to e46de6a Compare April 4, 2022 23:48
@HuiSF HuiSF merged commit 55f9ce6 into aws-amplify:feat/custom-pk Apr 5, 2022
@HuiSF HuiSF deleted the feat/custom-pk-model-identifier branch April 5, 2022 00:38
HuiSF added a commit that referenced this pull request Apr 6, 2022
* 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
HuiSF added a commit to HuiSF/amplify-flutter that referenced this pull request May 20, 2022
* 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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants