Skip to content

Conversation

@rustagir
Copy link
Contributor

@rustagir rustagir commented Oct 23, 2024

Pull Request Info

PR Reviewing Guidelines

JIRA - https://jira.mongodb.org/browse/DOCSP-44647
Staging - https://deploy-preview-55--docs-mongoid.netlify.app/add-existing/

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?

@netlify
Copy link

netlify bot commented Oct 23, 2024

Deploy Preview for docs-mongoid ready!

Name Link
🔨 Latest commit b876e64
🔍 Latest deploy log https://app.netlify.com/sites/docs-mongoid/deploys/672123690d0c84000813a496
😎 Deploy Preview https://deploy-preview-55--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.

Copy link
Contributor

@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.

very solid. some suggestions and questions

-------------------

To start using {+odm+} in an existing Sinatra application, you can follow
the same steps described in the
Copy link
Contributor

Choose a reason for hiding this comment

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

S:

Suggested change
the same steps described in the
the steps described in the

#. Create a ``config/mongoid.yml`` configuration file and specify your
connection target.

#. Create an application file and load your configuration file.
Copy link
Contributor

Choose a reason for hiding this comment

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

Q: This is something users will know how to do?

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 can link to the example in the QS

-----------------

You can add {+odm+} to an existing Rails application to run alongside
other ActiveRecord adapters. To use a combination of adapters, you
Copy link
Contributor

Choose a reason for hiding this comment

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

S: looks to me like there's "Active Record" (the framework) and ActiveRecord (the module). Not sure which one is appropriate throughout this page.

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 think I'll replace with the spaced version, as I am using it the same way as Mongoid

gem 'mongoid'

To use {+odm+} as the *only* database adapter, remove or comment out any
RDBMS libraries such as ``sqlite`` or ``pg`` listed in the ``Gemfile``.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
RDBMS libraries such as ``sqlite`` or ``pg`` listed in the ``Gemfile``.
RDBMS libraries listed in the ``Gemfile``, such as ``sqlite`` or ``pg``.

Comment on lines 106 to 108
To verify the contents of ``rails/all`` for Rails 7, see the
:github:`rails GitHub repository
</rails/rails/blob/7-0-stable/railties/lib/rails/all.rb>`.
Copy link
Contributor

Choose a reason for hiding this comment

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

S: tip admonition

Suggested change
To verify the contents of ``rails/all`` for Rails 7, see the
:github:`rails GitHub repository
</rails/rails/blob/7-0-stable/railties/lib/rails/all.rb>`.
.. tip:: rails/all Components
To verify the contents of ``rails/all`` for Rails 7, see the
:github:`rails GitHub repository. </rails/rails/blob/7-0-stable/railties/lib/rails/all.rb>`

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 actually think we can just delete this note.

If your application already uses models, you must adjust them to
migrate from using ActiveRecord to {+odm+}.

ActiveRecord models derive from ``ApplicationRecord`` and do not have
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
ActiveRecord models derive from ``ApplicationRecord`` and do not have
ActiveRecord models derive from the ``ApplicationRecord`` class and do not have

Comment on lines 156 to 160
ActiveRecord models derive from ``ApplicationRecord`` and do not have
column definitions. {+odm+} models generally have no superclass but must
include the ``Mongoid::Document`` attribute. {+odm+} models usually
define fields explicitly. You can also use :ref:`dynamic fields
<mongoid-dynamic-fields>` instead of explicit field definitions.
Copy link
Contributor

Choose a reason for hiding this comment

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

S: is there a way to tie these four sentences together more? i'm guessing it's explaining how you need to change your models to use Mongoid, but it reads more like a list--in which case, maybe just make it an actual list?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

will workshop

has_many :comments, dependent: :destroy
end

Or, you can define the ``Post`` model by using dynamic fields, as shown
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
Or, you can define the ``Post`` model by using dynamic fields, as shown
Instead of using [blank], you can define the ``Post`` model by using dynamic fields, as shown

Comment on lines 199 to 200
{+odm+} does not use ActiveRecord migrations, because MongoDB does not
require a defined schema before you can store data.
Copy link
Contributor

Choose a reason for hiding this comment

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

S: maybe my own ignorance, but this feels tacked on to the previous section instead of a logical continuation, so maybe a note admonition would be useful. maybe even move to the next section (or delete altogether), since it makes this same point.

Suggested change
{+odm+} does not use ActiveRecord migrations, because MongoDB does not
require a defined schema before you can store data.
.. note:: ActiveRecord Migrations
{+odm+} does not use ActiveRecord migrations, because MongoDB does not
require a defined schema before you can store data.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Deleted

connect to a MongoDB cluster hosted on MongoDB Atlas, and perform
read and write operations on the data in your cluster.

To learn how to migrate an existing application to use {+odm+}, see the
Copy link
Contributor

Choose a reason for hiding this comment

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

S: my ear says you either migrate to Mongoid or adapt the application to use Mongoid, but not a combo

Copy link
Contributor Author

Choose a reason for hiding this comment

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

THis is meant to differentiate between starting a new app or integrating Mongoid into your existing app. I can make that more clear

@rustagir rustagir requested a review from mongoKart October 24, 2024 14:49
Copy link
Contributor

@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.

looks great! a few small suggestions

:ref:`Sinatra Quick Start <mongoid-quick-start-sinatra>` guide.

The following steps describe how to add {+odm+} to a Sinatra application:
the following steps:
Copy link
Contributor

Choose a reason for hiding this comment

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

S: change line to "perform the following steps:" or something similar to avoid double "follow"

Open the ``config/application.rb`` file and examine the contents. If the
file uses the ``require "rails/all"`` statement to load all Rails components,
delete this statement. You must add a separate ``require`` statement
for each Rails component, as shown the following sample
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
for each Rails component, as shown the following sample
for each Rails component, as shown in the following sample

Even though {+odm+} supports a superset of Active Record associations,
the way that model references are stored in collections is different between
{+odm+} and ActiveRecord.
{+odm+} and Active Record.
Copy link
Contributor

Choose a reason for hiding this comment

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

S: "{+odm+} and Active Record store model references in collections differently."

@rustagir rustagir merged commit 5328c20 into mongodb:standardized Oct 29, 2024
1 check failed
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