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-40800: Specify documents to return #24

Open
wants to merge 9 commits into
base: cpp-standardization
Choose a base branch
from

Conversation

norareidy
Copy link
Collaborator

@norareidy norareidy commented Jun 27, 2024

Pull Request Info

PR Reviewing Guidelines

JIRA - https://jira.mongodb.org/browse/DOCSP-40800
Staging - https://preview-mongodbnorareidy.gatsbyjs.io/cpp-driver/DOCSP-40800-limit-sort-skip/read/specify-documents-to-return/

This PR also adds code comments to the retrieve.cpp file.

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?
  • Are the facets and meta keywords accurate?

Copy link

@shuangela shuangela left a comment

Choose a reason for hiding this comment

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

Good job! Some questions and nits below. Let me know if you have any clarifying questions/comments.

}

{
// Retrieves 5 matching documents, skips the first 10 results, and sorts by ascending "name" order

Choose a reason for hiding this comment

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

S: In the comment, add that the documents have a "cuisine" value of "Italian" to match previous comments


The examples in this guide use the ``restaurants`` collection in the ``sample_restaurants``
database from the :atlas:`Atlas sample datasets </sample-data>`. To access this collection
from your C++ application, instantiate a ``client`` that connects to an Atlas cluster

Choose a reason for hiding this comment

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

Suggested change
from your C++ application, instantiate a ``client`` that connects to an Atlas cluster
from your C++ application, instantiate a `` mongocxx::client`` that connects to an Atlas cluster

Q: Should client be changed to mongocxx::client?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

yes, good catch!


To specify the maximum number of documents returned from a read operation, create
an instance of the ``mongocxx::options::find`` class and set its ``limit`` field.
Then, pass your ``mongocxx::options::find`` instance as a parameter to the

Choose a reason for hiding this comment

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

nit: change parameter to argument

applies to all

The preceding example returns the first five documents matched by the query
according to their :manual:`natural order </reference/glossary/#std-term-natural-order>`
in the database. The following section describes how to return the documents
in a specified sort order.

Choose a reason for hiding this comment

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

nit: specified order rather than specified sort order

parameter to the ``find()`` method.


To set the ``sort`` field, call the ``sort()`` setter method on your ``mongocxx::options::find``

Choose a reason for hiding this comment

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

Q: You used "setter method" here when you did not use it above (the limit() function). Is there a difference between these two?

Choose a reason for hiding this comment

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

Based on the API docs, seems like the find() method and limit() are the same type of method, so adding setter might not clarify/help this sentence.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

sort() and limit() are setter methods, since they set fields of a mongocxx::options::find object; find() is not. But I can remove the word "setter" here if it's confusing

number of sorted documents to return, skipping a specified number of
documents before returning.

The following example returns documents that have a ``cuisine`` value of

Choose a reason for hiding this comment

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

Suggested change
The following example returns documents that have a ``cuisine`` value of
The following example returns ``5`` documents that have a ``cuisine`` value of

documents before returning.

The following example returns documents that have a ``cuisine`` value of
``"Italian"``. The results are sorted in ascending ``name`` value order,

Choose a reason for hiding this comment

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

Suggested change
``"Italian"``. The results are sorted in ascending ``name`` value order,
``"Italian"``. The results are sorted in ascending order by ``name`` field value,


The order in which you call these methods doesn't change the documents
that are returned. The driver automatically reorders the calls to perform the
sort and skip operations first, and the limit operation afterward.

Choose a reason for hiding this comment

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

Question: Do sort and skip happen at the same time? Which one happens first relative to the other? I think that the order they happen in would change the results of the operation.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Sort happens first, then skip!


To learn more about any of the methods or types discussed in this
guide, see the following API documentation:

Choose a reason for hiding this comment

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

Suggested change
- `mongocxx::options::find <{+api+}/classmongocxx_1_1v__noabi_1_1options_1_1find.html>`__

To learn more about any of the methods or types discussed in this
guide, see the following API documentation:

- `find() <{+api+}/classmongocxx_1_1v__noabi_1_1collection.html#ada76e1596a65c7615e7af7d34a8140d6>`__

Choose a reason for hiding this comment

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

S: add full namespace (mongocxx::...) to all the methods

applies to all

@norareidy norareidy requested a review from shuangela July 1, 2024 19:10
Copy link

@shuangela shuangela left a comment

Choose a reason for hiding this comment

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

LGTM!

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.

None yet

2 participants