GODRIVER-2698 Make Comment fields any-type on all options structs and setters #1454
Conversation
API Change Report./mongo/optionsincompatible changes(*AggregateOptions).SetComment: changed from func(string) *AggregateOptions to func(interface{}) *AggregateOptions ./x/mongo/driver/operationincompatible changes##(*Aggregate).Comment: changed from func(string) *Aggregate to func(./x/bsonx/bsoncore.Value) *Aggregate |
prestonvasquez
requested review from
blink1073 and
matthewdale
and removed request for
a team
| // Unconditionally send a limit to make sure only one document is returned and the cursor is not kept open | ||
| // by the server. | ||
| findOpts = append(findOpts, options.Find().SetLimit(-1)) |
There was a problem hiding this comment.
Optional: The explicit indexing is somewhat confusing at first and seems brittle. Is there a reason to do that vs using append?
There was a problem hiding this comment.
The append method causes the following linting error:
mongo/collection.go:1667:14: append to slice `findOpts` with non-zero initialized length (makezero)
findOpts = append(findOpts, &options.FindOptions{
^
mongo/collection.go:1685:13: append to slice `findOpts` with non-zero initialized length (makezero)
findOpts = append(findOpts, options.Find().SetLimit(-1))
There was a problem hiding this comment.
@matthewdale Looking at this more closely, I believe that his solution has a pretty insidious bug. Namely if a user passes options like this to the FindOne function: [op1, nil, op2], you would get a runtime fatal. I've updated the PR with a test for this.
prestonvasquez
dismissed stale reviews from matthewdale and blink1073
via
ea99f9b
mongo/options/aggregateoptions.go
Outdated
| // A string that will be included in server logs, profiling logs, and currentOp queries to help trace the operation. | ||
| // The default is nil, which means that no comment will be included in the logs. |
There was a problem hiding this comment.
We should update the comments to describe the expected value types. For example, Comment fields that are already interface{} have the following documentation:
A string or document that will be included in server logs, profiling logs, and currentOp queries to help trace the operation. The default is nil, which means that no comment will be included in the logs.
That suggestion applies to all doc comments for the updated Comment fields.
|
@blink1073 @matthewdale Could you re-review this PR? |
prestonvasquez
dismissed stale reviews from matthewdale and blink1073
via
1316751
GODRIVER-2698
Summary
Update all comment fields on option structs to accept an "any" type that is later parsed to be either a document or a string.
Background & Motivation
The Comment field on most options structs is an interface. However, on a few structs, Comment is a string because that type was required in an earlier form of the specification. Update all Comment fields and corresponding SetComment setters to be type any.