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

chore(datastore): add query & mutate impl #4858

Merged
merged 14 commits into from May 15, 2024

Conversation

Equartey
Copy link
Contributor

@Equartey Equartey commented May 8, 2024

Description of changes:
Adds Swift FlutterApiPlugin query and mutate implementations.

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

@Equartey Equartey requested a review from a team as a code owner May 8, 2024 17:35
@Equartey Equartey force-pushed the chore/flutter-api-query-mutate branch from 73d2d08 to ea0871d Compare May 10, 2024 17:34
@@ -35,7 +35,7 @@ class HubEventElementWithMetadata<M extends Model> extends HubEventElement<M> {
final int version;

/// The last time the model was updated locally, in seconds since epoch.
final int lastChangedAt;
final int? lastChangedAt;
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 part of the public API so it is technically a breaking change. Was this intended to be part of the public API? Is this a class that customers use (is there a use case in which they might use it)?

Ideally we should try to find a way to make this non-breaking. If that is not feasible and this was never intended to be public was can discuss deprecating this or marking it as internal.

@@ -63,7 +63,7 @@ class DataStoreHubEventStreamHandlerTests: XCTestCase {
waitForExpectations(timeout: 1.0)
// cancellation needed to make sure that Hub token is invalidated to
// prevent collisions between tests
hubHandler.onCancel(withArguments: nil)
let _ = hubHandler.onCancel(withArguments: nil)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Q: what is the purpose of this change? Is this the norm in swift or enforced via linting?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Swift linter complains about the value was not bing used. This is a common pattern to satisfy the linter rule, while denoting the return value is not used. Relevant SO

pod 'Amplify', path: '../../ios/internal/'
pod 'AWSPluginsCore', path: '../../ios/internal/'
pod 'AWSDataStorePlugin', path: '../../ios/internal/'
pod 'Amplify', path: '../../ios/internal/', :inhibit_warnings => true
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Q: What is the purpose of this change?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

CI logs were showing tons of lint warnings for these files, but since we they are from Amplify Swift we don't care. We're not going to modify these files. Adding this line prevents warning messages in CI, making those logs more readable.

@@ -63,7 +63,7 @@ class DataStoreHubEventStreamHandlerTests: XCTestCase {
waitForExpectations(timeout: 1.0)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not related to this PR, but we should remove these from generated files in .gitattributes. Everything under **/example/ios/** is treated as generated code since the majority of it is generated by flutter during flutter create. We should remove unit_tests from this.

Could you make that change in this feature branch (can be in a follow up PR) since there will probably be quite a few PRs that will updates native tests?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good call out, I added that to this branch.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you

Copy link
Contributor

@Jordan-Nelson Jordan-Nelson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Change LGTM. We may have to make changes to for the breaking change but we can discuss offline and handle that in a follow up.

@@ -63,7 +63,7 @@ class DataStoreHubEventStreamHandlerTests: XCTestCase {
waitForExpectations(timeout: 1.0)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you

Base automatically changed from chore/flutter-api-subscribe to feat/datastore-spm-migration May 13, 2024 16:51
@Equartey Equartey force-pushed the chore/flutter-api-query-mutate branch from 64689fd to c1b3b7a Compare May 13, 2024 16:56
@Equartey Equartey merged commit ab777f2 into feat/datastore-spm-migration May 15, 2024
21 of 22 checks passed
@Equartey Equartey deleted the chore/flutter-api-query-mutate branch May 15, 2024 17:03
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.

None yet

3 participants