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.

Sign up for GitHub

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

Jump to bottom

fix(api): support query predicate Temporal.DateTime, fix integ tests #508

Merged
merged 5 commits into from
Jun 4, 2020
Merged

fix(api): support query predicate Temporal.DateTime, fix integ tests #508

merged 5 commits into from
Jun 4, 2020

Conversation

lawmicha
Copy link
Contributor

@lawmicha lawmicha commented Jun 2, 2020

Issue #, if available:

Description of changes:

  • Retest the integration tests to get them working with the updated schema and Temporal classes
  • Fixed QueryPredicate to GraphQLValue logic, missing Temporal.DateTime conversion

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@lawmicha lawmicha changed the title bug(api): support query predicate Temporal.DateTime, fix integ tests fix(api): support query predicate Temporal.DateTime, fix integ tests Jun 2, 2020
@lawmicha lawmicha force-pushed the fix-api-temporaldatetime branch from ca6a803 to 9860fd1 Compare June 2, 2020 16:10
@lawmicha lawmicha requested review from wooj2, drochetti and palpatim June 2, 2020 21:32
drochetti
drochetti suggested changes Jun 4, 2020
View reviewed changes
Copy link
Contributor

@drochetti drochetti left a comment

Choose a reason for hiding this comment

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

Thanks for fixing the tests. I requested to change every occurrence of Temporal.DateTime(Date()) and Temporal.DateTime.now() to simply .now().

Other than that, lgtm!

AmplifyPlugins/API/AWSAPICategoryPluginFunctionalTests/AnyModelIntegrationTests.swift Outdated
let originalPost = Post(title: "Post title",
content: "Original post content as of \(Date())",
createdAt: Date())
let originalPost = AmplifyTestCommon.Post(title: "Post title",
Copy link
Contributor

Choose a reason for hiding this comment

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

is there a conflict with Post so you needed to use the fully qualified name? I wonder if we could remove the conflict instead.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

i did some clean up here and removed the non-Model based Blog/Post/Comment types which have tests that were also removed (they were disabled, other Model based integ tests cover the same scenario) so that we can remove the need for the fully qualified name here and rest of the files

@lawmicha lawmicha force-pushed the fix-api-temporaldatetime branch from c9e9ae8 to 1d22a27 Compare June 4, 2020 15:28
@lawmicha lawmicha requested a review from drochetti June 4, 2020 15:35
@lawmicha lawmicha self-assigned this Jun 4, 2020
@lawmicha lawmicha added the api Issues related to the API category label Jun 4, 2020
drochetti
drochetti reviewed Jun 4, 2020
View reviewed changes
Copy link
Contributor

@drochetti drochetti left a comment

Choose a reason for hiding this comment

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

just one last request: there are some minor indentation issues. I wonder if SwiftFormat is running for this target?

Anyway, other than that, it lgtm!

AmplifyTestCommon/Models/schema.graphql
@@ -20,7 +20,7 @@ type Comment @model {
id: ID!
content: String!
createdAt: AWSDateTime!
post: Post @connection(name: "PostComment")
post: Post! @connection(name: "PostComment")
Copy link
Contributor

Choose a reason for hiding this comment

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

nice! :)

AmplifyPlugins/API/AWSAPICategoryPluginFunctionalTests/AnyModelIntegrationTests.swift Outdated
let originalPost = Post(title: "Post title",
content: "Original post content as of \(Date())",
createdAt: Date())
createdAt: .now())
Copy link
Contributor

Choose a reason for hiding this comment

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

indentation

Copy link
Contributor Author

@lawmicha lawmicha Jun 4, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

the line above

content: "Original post content as of \(Date())",

with the string interpolation is what's causing xcode to have some sort of indentation bug there. when i remove it and CMD+I on the createdAt line, it places it in line with the title and content parameters

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed by creating some variables

@lawmicha
Copy link
Contributor Author

lawmicha commented Jun 4, 2020

just one last request: there are some minor indentation issues. I wonder if SwiftFormat is running for this target?

Anyway, other than that, it lgtm!

Thanks for looking, i saw some indentation issues too when i ran CMD+i, let me take a look again before i merge it in

@lawmicha lawmicha merged commit 3d3c038 into master Jun 4, 2020
@lawmicha lawmicha deleted the fix-api-temporaldatetime branch June 4, 2020 19:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@drochetti drochetti drochetti approved these changes

@wooj2 wooj2 Awaiting requested review from wooj2

@palpatim palpatim Awaiting requested review from palpatim

Assignees

@lawmicha lawmicha

Labels
api Issues related to the API category
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@lawmicha @drochetti