Skip to content

Conversation

rustagir
Copy link
Contributor

@rustagir rustagir commented Dec 20, 2024

Pull Request Info

PR Reviewing Guidelines

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

  1. persist
  2. cache
  3. async

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

netlify bot commented Dec 20, 2024

Deploy Preview for docs-mongoid ready!

Name Link
🔨 Latest commit 1c68896
🔍 Latest deploy log https://app.netlify.com/sites/docs-mongoid/deploys/678018483ef9310008a6f35a
😎 Deploy Preview https://deploy-preview-80--docs-mongoid.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@rustagir rustagir marked this pull request as ready for review December 20, 2024 18:10
Copy link
Collaborator

@mayaraman19 mayaraman19 left a comment

Choose a reason for hiding this comment

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

Looks pretty good to me, I just left a couple comments

Comment on lines 44 to 56
executed synchronously on the caller's thread. The following list
describes possible scenarios that this might occur depending on when the
query results are being accessed:

1. If the scheduled asynchronous task has been executed, the
results are returned.

#. If the task has been started, but hasn't completed, the caller's
thread blocks until the task is finished.

#. If the task has not been started yet, it is removed from the
execution queue, and the query is executed synchronously on the
caller's thread.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Q: is this list exhaustive?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am not sure - taken from the original text. But the introduction to this list doesn't give the impression that it is exhuastive "possible scenarios"

Comment on lines 44 to 46
executed synchronously on the caller's thread. The following list
describes possible scenarios that this might occur depending on when the
query results are being accessed:
Copy link
Collaborator

Choose a reason for hiding this comment

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

S:

Suggested change
executed synchronously on the caller's thread. The following list
describes possible scenarios that this might occur depending on when the
query results are being accessed:
executed synchronously on the caller's thread. The following list
describes possible scenarios that this might occur:

You can then access the results of the queries in your view as you
normally do for synchronous queries.

Even if a query is scheduled for asynchronous execution, it might be
Copy link
Collaborator

Choose a reason for hiding this comment

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

S: active voice

Suggested change
Even if a query is scheduled for asynchronous execution, it might be
Even if you schedule a query for asynchronous execution, it might be

Comment on lines +71 to +73
on the current thread, blocking as required. Therefore, calling
``load_async`` on a ``Criteria`` instance in this situation is similar
to calling the ``to_a`` method to force query execution.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Q: I'm not familiar with this method - how is this similar?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

to_a performs a conversion to an array of documents, i believe that this forces query execution because it requires the query to return documents to convert those documents to an array

Copy link
Contributor

Choose a reason for hiding this comment

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

Correct, if you don't call to_a you're not actually sending the command to the server and just returning an instance of Mongoid::Criteria instead

Mongo::QueryCache.enabled = false
end

Cache the Result of the first Method
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
Cache the Result of the first Method
Cache the Result of the ``first`` Method

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Avoiding using monospace in titles per style guide

Comment on lines 86 to 87
cached results from ``all``. However, because of the sort that {+odm+}
applies to the second call, both methods query the database and
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
cached results from ``all``. However, because of the sort that {+odm+}
applies to the second call, both methods query the database and
cached results from ``all``. However, because {+odm+} applies
a sort to the second call, both methods query the database and

Comment on lines +90 to +96
To use the cached results when calling the ``first`` method, call
``all.to_a.first`` on the model class, as shown in the following example
code:

.. code-block::

Band.all.to_a.first
Copy link
Collaborator

Choose a reason for hiding this comment

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

S: might just be my unfamiliarity with to_a, but can you briefly explain what this code does?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

to_a stores the documents in an array, so when first applies the sort, its just on the array in memory instead of executing a new query. I can add this info to the page.

Copy link
Contributor

@gmiller-mdb gmiller-mdb left a comment

Choose a reason for hiding this comment

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

Great work, requesting changes just because I left a few suggestions!

Comment on lines 47 to 55
1. If the scheduled asynchronous task has been executed, the
results are returned.

#. If the task has been started, but hasn't completed, the caller's
thread blocks until the task is finished.

#. If the task has not been started yet, it is removed from the
execution queue, and the query is executed synchronously on the
caller's thread.
Copy link
Contributor

Choose a reason for hiding this comment

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

S: Can you convert some of these from passive to active voice?

- Example: ``Band.where(name: 'Daft Punk').create``

- ``create!``: Saves a model instance to MongoDB or raises an exception
if a validation error occurs.
Copy link
Contributor

Choose a reason for hiding this comment

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

s: remove this period as it is a sentence fragment. applies to all items in this list.

Suggested change
if a validation error occurs.
if a validation error occurs

Copy link
Contributor Author

Choose a reason for hiding this comment

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

removed periods from all lists except delete section as one entry has two sentences.

You can use the following methods to update documents based on your
query criteria:

- ``update``: Updates attributes of the first matching document.
Copy link
Contributor

Choose a reason for hiding this comment

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

s: same suggestion as above.

Suggested change
- ``update``: Updates attributes of the first matching document.
- ``update``: Updates attributes of the first matching document

Comment on lines 43 to 45
query cache for Rack web requests and Active Job job runs. See the
:ref:`Query Cache Rack Middleware <query-cache-middleware>` section of
the Configuration guide for instructions.
Copy link
Contributor

Choose a reason for hiding this comment

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

s: Link text should begin with an explanation of the purpose of the link: https://www.mongodb.com/docs/meta/style-guide/style/links-and-cross-references/#make-link-text-clear

Suggested change
query cache for Rack web requests and Active Job job runs. See the
:ref:`Query Cache Rack Middleware <query-cache-middleware>` section of
the Configuration guide for instructions.
query cache for Rack web requests and Active Job job runs. For instructions
on automatically enabling the query cache, see the :ref:`Query Cache Rack Middleware <query-cache middleware>` section of the configuration guide.


- ``update``: Updates attributes of the first matching document.

- Example: ``Band.where(name: 'Sundown').update(label: 'ABC Records')``
Copy link
Contributor

Choose a reason for hiding this comment

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

q: up to you, but was wondering why this is a long list instead of a table, given that each method has an example.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We prev got feedback that people think the code in tables is too small to read

@rustagir rustagir requested a review from gmiller-mdb January 9, 2025 18:41
Copy link
Contributor

@gmiller-mdb gmiller-mdb left a comment

Choose a reason for hiding this comment

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

LGTM!

@rustagir rustagir merged commit 39e5eee into mongodb:standardized Jan 10, 2025
4 of 5 checks passed
rustagir added a commit that referenced this pull request Jan 28, 2025
* DOCSP-42732: qs

* fix

* wip

* remove action

* NR suggestion

* DOCSP-42732: qs download

* wip

* adapt for sinatra

* vale fix

* snooty landing page

* MW PR fixes 1

* DOCSP-42733: atlas prep qs

* MW PR fixes 1

* DOCSP-42735: configure cxn

* small fix - vale

* JS PR fixes 1

* DOCSP-44008: read/write sinatra quickstart

* fixes

* NR PR fixes 1

* DOCSP-43961: rails qs

* vale

* ordering

* small fix

* MW PR fixes 1

* small fixes

* MW PR fixes 2

* DOCSP-44647: add to existing app

* fix vale action

* vale fixes

* depth

* MW PR fixes 1

* fixes

* DOCSP-42745: interact with data drawer

* tags

* fix vale action

* remove extra word

Co-authored-by: Nora Reidy <nora.reidy@mongodb.com>

* DOCSP-42753: specify query part 1

* vale

* title

* code edits

* MW PR fixes 2

* DOCSP-44849: modify results

* vale

* JS PR fixes 1

* fix

* fix

* list fixes

* MW PR fixes 1

* DOCSP-44954: scoping

* add landing page

* link

* vale

* highlighting

* DOCSP-44821: specify a query pt 2

* MR PR fixes 1

* wip

* wip

* wip

* small fixes

* DOCSP-42767 Aggregation (#57)

* DOCSP-42774: transactions

* vale

* link text

* MW PR fixes 1

* MR PR fixes 1

* DOCSP-45306: model data drawer

* DOCSP-45330: inheritance (WIP)

* MR PR fixes 2

* try using roles

* wip

* vale

* add label

* fixes

* fix

* small fixes - MW

* DOCSP-45358: documents

* fix

* wip

* wip

* DR tech review 1

* page fmt

* page fmt

* SA PR fixes 1

* MR PR fixes 1

* DR small fix

Co-authored-by: Dmitry Rybakov <160598371+comandeo-mongo@users.noreply.github.com>

* DOCSP-45360: nested attributes (#71)

* DOCSP-45360: nested attributes

* vale + fixes

* fixes

* NR PR fixes 1

* DOCSP-45362: text search (#72)

* DOCSP-45362: text search

* wip

* vale

* MM PR fixes 1

* DOCSP-45436 Field Behaviors page (#68)

* DOCSP-45363: validation (#73)

* DOCSP-45363: validation

* keywords

* wip

* SA PR fixes 1

* DOCSP-44794 Field Types (#69)

* DOCSP-45357 Sharding Configuration (#76)

* DOCSP-42762: Indexes (#74)

* DOCSP-45367 Associations pt. 1 (#79)

* DOCSP-45368: Persistence Configuration (#77)

* DOCSP-45361: callbacks (#75)

* DOCSP-45361: callbacks

* wip

* wip

* wip

* NR PR fixes 1

* DOCSP-45364: CRUD pt 1 (#81)

* checkpoint

* checkpoint 2

* woohoo first pass

* indent

* Edits

* updates

* vale chekcs

* RR PR fixes 1

* fix code file

* code fixes

* RM PR fixes 1

---------

Co-authored-by: rustagir <rea.rustagi@mongodb.com>

* DOCSP-45110: queries subsections (#80)

* DOCSP-45110: queries misc sections

* wip:

* vale

* MR PR fixes 1

* GM PR fixes 1

* DOCSP-46072 Associations part 2 (#82)

* DOCSP-46394: CRUD remaining sections (#83)

* DOCSP-46394: CRUD remaining sections

* vale fixes

* JS PR fixes 1

* DOCSP-46213: bump to rails 8 and remove old tuts (#84)

* DOCSP-45356: i&h + code doc (#86)

* DOCSP-45356: i&h + code doc

* remove contributing

* vale fixes

* link fix

* vale fixes + RM comment

* DOCSP-42770: release notes/whats new (#87)

* DOCSP-42770: release notes/whats new

* fixes

* fixes

* DOCSP-42773: api links (#88)

* DOCSP-42773: api links

* fix

* link fixes

* DOCSP-42772: compatibility (#90)

* DOCSP-42772: compatibility

* small fix

* small fix

* SA PR fixes 1

* delete files for old build system

* column width adjustment

* DOCSP-45366 Encryption (#89)

* DOCSP-42741: config pages (#91)

* DOCSP-42741: config

* wip

* wip

* some vale fixes

* RM PR fixes 1

* small fix

* DOCSP-46555: rails integration (#92)

* wip

* DOCSP-46555: rails integration

* RM PR fixes 1

* DOCSP-45359 External Resources (#94)

* add additional resources page

* edits

* feedback

* DOCSP-42743 Collection config (#95)

* DOCSP-42730: landing page (#96)

* DOCSP-42730: landing page

* MW PR fixes 1

* small fix

* small fix

* small fix

* DOCSP-46121: cleanup (#97)

* cleanup

* copy compat action

* redirects

* MW PR fixes 1

* add index section

* change vs in redirects

---------

Co-authored-by: Nora Reidy <nora.reidy@mongodb.com>
Co-authored-by: Jordan Smith <45415425+jordan-smith721@users.noreply.github.com>
Co-authored-by: Dmitry Rybakov <160598371+comandeo-mongo@users.noreply.github.com>
Co-authored-by: Maya Raman <maya.raman19@gmail.com>
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.

4 participants