-
Notifications
You must be signed in to change notification settings - Fork 67
SWIFT-355 Make option properties mutable #278
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
Conversation
kmahar
left a comment
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.
very nice! 🙂 just a couple of comments. please let me know if you have any questions about them!
| public var comment: String? | ||
|
|
||
| /// If a `CursorType` is provided, indicates whether it is `.tailable` or .`tailableAwait`. | ||
| private let tailable: Bool? |
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.
so I just realized there is something a little tricky that we need to handle in FindOptions.
we allow the user to provide a CursorType input to the initializer. CursorType is an enum with three possible values: nonTailable, tailable, and awaitData.
however, when we send that info to libmongoc in the MongoCollection.find method, we need to send it as two bools instead: tailable and await. currently, we decide the values of tailable and await in the FindOptions initializer here based on whatever CursorType the user specifies.
and then, we include tailable and await in our CodingKeys list, which lets us specify which values in FindOptions actually get sent to libmongoc.
currently, this works fine because we never change cursorType outside of the initializer.
however, now a user could do something like the following:
var options = FindOptions(cursorType: .tailable) // the initializer will set tailable=true, await=false
options.cursorType = .tailableAwait // now we should have tailable=true, await=true, but we didn't update their values!basically, we need a way to keep tailable and await in sync with cursorType.
I think the simplest way to fix this would be to make FindOptions.cursorType a computed property. you can read a little bit more about them on this page (scroll down to "Computed Properties" section.) take a read through that and then let's talk more in person later today about how we can use it here 🙂
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.
I think we can also leave tailable and await as let since we don't want to be modifying them directly but only through the new cursor type property.
edit: disregard this
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.
If we declare them with let, how would we modify them as needed in my example above?
97f7251 to
68941df
Compare
| self.batchSize = batchSize | ||
| self.collation = collation | ||
| self.comment = comment | ||
| // although this does not get encoded, we store it for debugging purposes |
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] this comment can be removed.
| public var comment: String? | ||
|
|
||
| /// If a `CursorType` is provided, indicates whether it is `.tailable` or .`tailableAwait`. | ||
| private let tailable: Bool? |
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.
I think we can also leave tailable and await as let since we don't want to be modifying them directly but only through the new cursor type property.
edit: disregard this
kmahar
left a comment
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.
suggestion for a couple more test cases to add. otherwise looks great!
kmahar
left a comment
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.
awesome job, LGTM 👏
patrickfreed
left a comment
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! great work on your first ticket
Makes all option properties mutable. Option properties are declared with 'var' instead of 'let'.