-
Notifications
You must be signed in to change notification settings - Fork 117
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(datastore): Add non-model type support for amplify-flutter #1459
Conversation
public static synchronized SchemaRegistry instance() { | ||
if (SchemaRegistry.instance == null) { | ||
SchemaRegistry.instance = new SchemaRegistry(); | ||
} | ||
|
||
return SchemaRegistry.instance; |
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.
From this use case of SchemaRegistry
, it looks like SchemaRegistry
should be a singleton. The original code didn't work and didn't break either as the original logic wasn't really depending on retrieving a valid model schema.
@@ -28,7 +28,7 @@ | |||
import com.amplifyframework.core.model.ModelAssociation; | |||
import com.amplifyframework.core.model.ModelField; | |||
import com.amplifyframework.core.model.ModelSchema; | |||
import com.amplifyframework.core.model.ModelSchemaRegistry; |
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.
What was the rationale behind renaming this class?
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.
Thanks for asking :)
ModelSchemaRegistry
not holds CustomTypeSchema
as well, the original name was leading by Model
which could be confusing, so updated to just SchemaRegistry
.
* @param modelSchema schema for the Model object | ||
* @param <T> type of the Model object. | ||
* @param <T> type of the Model object. |
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.
nit: some of the formatting looks like it needs to be corrected
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 formatting is automatically added by the IDE I think, which align parameter description block with each other for readability.
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
Issue #, if available: amplify-flutter #260
Description of changes:
ModelSchemaRegistry
support registeringCustomType
schemasSerializedModel
nestingSerializedCustomType
SerializedCustomType
for amplify-flutterTasks
ModelSchemaRegistry
toSchemaRegistry
CustomTypeSchema
,SerializedCustomType
and related classesSerializedCustomType
with amplify-flutter use casesaws-api-appsync
aws-datastore
core
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.