Skip to content
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

Docsp 30927 work with bson options #278

Merged
merged 7 commits into from
Jul 11, 2023

Conversation

nickldp
Copy link
Contributor

@nickldp nickldp commented Jul 10, 2023

Pull Request Info

PR Reviewing Guidelines

JIRA - https://jira.mongodb.org/browse/DOCSP-30927
Staging

Self-Review Checklist

  • Is this free of any warnings or errors in the RST?
  • Did you run a spell-check?
  • Did you run a grammar-check?
  • Are all the links working?

Copy link
Collaborator

@mongoKart mongoKart left a comment

Choose a reason for hiding this comment

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

Nice job! A few small changes

Comment on lines 217 to 218
the driver marshals and unmarshals BSON on any level, from
``collection`` to ``Client``.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Issue: I'm not sure what "any level" refers to. How many are there?

Suggestion: Can we be more specific about these levels?

Comment on lines 220 to 221
The following example demonstrates how the ``BSONOptions`` is set using
the ``Movies`` collection as an example.
Copy link
Collaborator

Choose a reason for hiding this comment

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

I: Two points from the Style Guide here: Avoid passive voice and Don't use "using" alone

S:

Suggested change
The following example demonstrates how the ``BSONOptions`` is set using
the ``Movies`` collection as an example.
The following example demonstrates how to set ``BSONOptions`` for
the ``Movies`` collection:

Comment on lines 220 to 221
The following example demonstrates how the ``BSONOptions`` is set using
the ``Movies`` collection as an example.
Copy link
Collaborator

Choose a reason for hiding this comment

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

I: In our other docs, we keep collection names lowercase (e.g. here)

S:

Suggested change
The following example demonstrates how the ``BSONOptions`` is set using
the ``Movies`` collection as an example.
The following example demonstrates how the ``BSONOptions`` is set using
the ``movies`` collection as an example.

Comment on lines 232 to 234
To learn more about the options and usage of ``BSONOptions``, visit the `API Documentation
<https://pkg.go.dev/go.mongodb.org/mongo-driver/mongo#example-Connect-BSONOptions>`_
for more information.
Copy link
Collaborator

Choose a reason for hiding this comment

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

I: This sentence can probably be shorter and a little clearer

S:

Suggested change
To learn more about the options and usage of ``BSONOptions``, visit the `API Documentation
<https://pkg.go.dev/go.mongodb.org/mongo-driver/mongo#example-Connect-BSONOptions>`_
for more information.
To learn more about ``BSONOptions``, see the `API Documentation
<https://pkg.go.dev/go.mongodb.org/mongo-driver/mongo#example-Connect-BSONOptions>`__.

coll := client.Database("sample_mflix").Collection("movies",
bsonOpts)

To learn more about the options and usage of ``BSONOptions``, visit the `API Documentation
Copy link
Collaborator

Choose a reason for hiding this comment

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

S: Put this sentence in a Tip box, since it's more supplementary than a natural conclusion to this section

Comment on lines 229 to 230
coll := client.Database("sample_mflix").Collection("movies",
bsonOpts)
Copy link
Collaborator

Choose a reason for hiding this comment

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

I: I think these lines should be dedented back to the beginning...

Suggested change
coll := client.Database("sample_mflix").Collection("movies",
bsonOpts)
coll := client.Database("sample_mflix").Collection("movies",
bsonOpts)

bsonOpts)

To learn more about the options and usage of ``BSONOptions``, visit the `API Documentation
<https://pkg.go.dev/go.mongodb.org/mongo-driver/mongo#example-Connect-BSONOptions>`_
Copy link
Collaborator

Choose a reason for hiding this comment

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

S: We almost always use source constants (in snooty.toml) to represent API links, making it easier and less error-prone. I would try using a source constant here.

@nickldp nickldp requested a review from mongoKart July 10, 2023 20:14
Comment on lines 226 to 230
bsonOpts := &options.BSONOptions{
UseJSONStructTags: true,
}
coll := client.Database("sample_mflix").Collection("movies",
bsonOpts)
Copy link
Collaborator

Choose a reason for hiding this comment

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

S: I'm not a Go expert, but structuring the code like the following seems more readable:

Suggested change
bsonOpts := &options.BSONOptions{
UseJSONStructTags: true,
}
coll := client.Database("sample_mflix").Collection("movies",
bsonOpts)
bsonOpts := &options.BSONOptions {
UseJSONStructTags: true
}
coll := client.Database("sample_mflix")
.Collection("movies", bsonOpts)

What do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agree with everything, but comma on line 227 is needed

Copy link
Collaborator

@mongoKart mongoKart left a comment

Choose a reason for hiding this comment

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

LGTM + 1 suggestion. Great job addressing the feedback!

@nickldp nickldp merged commit 41298c8 into mongodb:master Jul 11, 2023
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.

2 participants