Skip to content

Conversation

@patrickfreed
Copy link
Contributor

This is part of a series of reviews that make up SWIFT-178

This PR adds Decodable conformance to options and result types that will eventually be decoded from JSON spec test files. It also improves the error parser to be more flexible when decoding from server replies.

let decoder = BSONDecoder()
var insertedIds = result?.insertedIds

var bulkWriteErrors: [BulkWriteError]?
Copy link
Contributor Author

Choose a reason for hiding this comment

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

When using fail points, sometimes empty (but non-nil) writeErrors and writeConcernErrors arrays are returned for parsing. The way this function was written assumed that wasn't possible and has been rewritten to accommodate those cases.

@patrickfreed patrickfreed marked this pull request as ready for review May 15, 2019 16:00
@patrickfreed patrickfreed requested review from kmahar and mbroadst May 15, 2019 16:00
Copy link
Contributor

@kmahar kmahar left a comment

Choose a reason for hiding this comment

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

looks good to me, just one thing to check re: docs

/// Map of the index of the operation to the id of the upserted document.
public let upsertedIds: [Int: BSONValue]

private enum CodingKeys: CodingKey {
Copy link
Contributor

Choose a reason for hiding this comment

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

it always annoys me that the synthesized code is all or none and you can't use the synthesized coding keys if you use a custom init(from:) implementation...

Copy link
Member

@mbroadst mbroadst left a comment

Choose a reason for hiding this comment

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

LGTM, just two questions

@patrickfreed patrickfreed merged commit 506db1b into master May 21, 2019
@patrickfreed patrickfreed deleted the SWIFT-178/retryable-writes-decodable-options branch July 12, 2019 17:05
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