Commit generated code for tests to provide more information to reviewers #17
Conversation
dylanahsmith
force-pushed
the
commit-generated-code
branch
from
70e678e to
14be2cb
Compare
|
What should I be reviewing here? The generated sample code, or the fact that it is present in the repo? |
|
The fact that it is present? |
| guard let value = value as? String else { | ||
| throw SchemaViolationError(type: UnknownEntry.self, field: fieldName, value: fieldValue) | ||
| } | ||
| return iso8601DateParser.date(from: value)! |
There was a problem hiding this comment.
🚨 If the date string is in an unexpected format, this will cause our clients to die at runtime with a fatal error that is out of their control.
Instead, the date parsing could be rolled into the guard clause so that failure to parse becomes a SchemaViolationError as well.
There was a problem hiding this comment.
The code to serialize and deserialize a date string is specified to the generator, so this would only need to be improved for the example. It would be worth checking to see what we do for our shopify ios app though.
There was a problem hiding this comment.
Looks like we use essentially the same code:
return GraphQL.iso8601DateParser.date(from: value)!
Re: #12 (comment)
Problem
It is harder to see what effect changes to the generator has without seeing an example of what is generated.
Solution
We generate code as part of the test suite which we can check in to git to see a diff of changes to those files. This will make it easier to see what effect changes to the generator will have.