-
Notifications
You must be signed in to change notification settings - Fork 918
WRITING-26459 Add v2 Migration Guide #1766
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
Co-authored-by: Qingyang Hu <[email protected]>
Co-authored-by: Qingyang Hu <[email protected]>
Co-authored-by: Julia Tazin <[email protected]>
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.
A few formatting suggestions
docs/migration-2.0.md
Outdated
- `UnmarshalExtJSONWithRegistry` | ||
- `UnmarshalExtJSONWithContext` | ||
|
||
For example, a boolean type can be stored in the database as a BSON boolean, int32, or int64. Given a registry: |
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.
For example, a boolean type can be stored in the database as a BSON boolean, int32, or int64. Given a registry: | |
For example, a boolean type can be stored in the database as a BSON `Boolean`, `Int32`, or `Int64`. Given a registry: |
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.
Go native types are not capitalized.
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 regard "boolean", "in32", "int64", or "true" are general concepts, that don't need to be denoted.
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.
@prestonvasquez isn't this specifically talking about the BSON types rather than the native types? This suggestion was based on the code sample below which has bsontype.Boolean
, bsontype.Int32
, and bsontype.Int64
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.
BSON type references should also not be capitalized.
I think we should either leave them as-is or reference the package type, as you suggest (i.e. bsontype.Boolean
).
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.
Ok, I'll defer to your judgement here. I noticed that it was capitalized in the code sample, which is why I suggested it, but if it doesn't make sense, let's keep it as-is. Feel free to resolve this one :)
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've updated the guide to use phrasing from the BSON specifications.
docs/migration-2.0.md
Outdated
bsoncodec.ValueDecoderFunc(lenientBoolDecoder)) | ||
``` | ||
|
||
For our custom decoder with such a registry, BSON int32 or int64 values are considered "true" if they are non-zero. |
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.
For our custom decoder with such a registry, BSON int32 or int64 values are considered "true" if they are non-zero. | |
For our custom decoder with such a registry, BSON `Int32` or `Int64` values are considered `true` if they are non-zero. |
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.
We can keep the int32 and int64 lowercase as discussed in the previous thread.
But I'm guessing we should remove the quotation around "true" though. It's probably best to format it as code
(based off of the convention I'm noticing in the rest of the doc when it comes to values, for example "to differentiate between an ID of 0
and no ID").
docs/migration-2.0.md
Outdated
|
||
### ValueMarshaler | ||
|
||
The `MarshalBSONValue` method of the `ValueMarshaler` interface is only required to return a byte type value representing the BSON type to avoid importing the Go driver package. |
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.
What's the name of the package that no longer needs to be imported?
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.
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've updated at 60af8e3
Co-authored-by: Julia Tazin <[email protected]>
Co-authored-by: Julia Tazin <[email protected]>
Co-authored-by: Julia Tazin <[email protected]>
Co-authored-by: Julia Tazin <[email protected]>
Co-authored-by: Julia Tazin <[email protected]>
Co-authored-by: Julia Tazin <[email protected]>
Co-authored-by: Qingyang Hu <[email protected]>
Co-authored-by: Rea Rustagi <[email protected]>
Co-authored-by: Rea Rustagi <[email protected]>
Co-authored-by: Rea Rustagi <[email protected]>
Co-authored-by: Rea Rustagi <[email protected]>
Co-authored-by: Rea Rustagi <[email protected]>
Co-authored-by: Rea Rustagi <[email protected]>
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.
looks good, I still suggest standardizing the way different code components (methods/types/etc) are displayed, but it is a low priority and users should understand
```go | ||
client, err := mongo.NewClient(options.Client()) | ||
if err != nil { | ||
log.Fatalf("failed to create client: %v", err) | ||
} | ||
|
||
if err := client.Connect(context.TODO()); err != nil { | ||
log.Fatalf("failed to connect to server: %v", err) | ||
} | ||
``` | ||
|
||
```go | ||
client, err := mongo.Connect(options.Client()) | ||
if err != nil { | ||
log.Fatalf("failed to connect to server: %v", err) | ||
} | ||
``` |
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.
S: add the v1 and v2 code comments for clarity
@rustagir agreed, do you have any specific suggestions on how to do that? |
Re: #1766 (comment) In the docs we usually follow the convention to identify code components eg.
And so on. But I am leaving this as an optional formatting suggestion for the team. |
API Change ReportNo changes found! |
WRITING-26459
Summary
Add the v2 migration guide to docs
Background & Motivation