Skip to content

Conversation

rustagir
Copy link
Contributor

@rustagir rustagir commented Nov 25, 2024

Pull Request Info

PR Reviewing Guidelines

JIRA - https://jira.mongodb.org/browse/DOCSP-45363
Staging - https://deploy-preview-73--docs-mongoid.netlify.app/data-modeling/validation/

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 Nov 25, 2024

Deploy Preview for docs-mongoid ready!

Name Link
🔨 Latest commit 7ae1b71
🔍 Latest deploy log https://app.netlify.com/sites/docs-mongoid/deploys/674dc4945c76090008aacd26
😎 Deploy Preview https://deploy-preview-73--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.

@stephmarie17 stephmarie17 self-requested a review November 26, 2024 17:31
Copy link

@stephmarie17 stephmarie17 left a comment

Choose a reason for hiding this comment

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

Nice work! Mostly just nits but a few questions 💬

Comment on lines 132 to 133
whether a field value matches a regular expression. Specify the regular
expression to the ``with`` option.

Choose a reason for hiding this comment

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

[q] Not super familiar with Mongoid but is this a common way to explain setting options? Specify to (instead of with)? It reads a little awkwardly, would something like this be accurate:

Suggested change
whether a field value matches a regular expression. Specify the regular
expression to the ``with`` option.
whether a field value matches a regular expression. Use the ``with`` option to specify the regular expression.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

updated


The ``comparison`` helper supports the following options:

- ``greater_than``: The value must be greater than the supplied value.

Choose a reason for hiding this comment

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

[q] Another Mongoid question 😄 In the linked Active Record docs, they format the options like :greater-than. Should these docs use this format also or not necessary?

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 don't think it's necessary, as we provide an example that shows the symbol syntax!

Comment on lines 163 to 164
document based on whether a field value is in or not in a specified list
of values. Specify the list to the ``in`` option.

Choose a reason for hiding this comment

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

[s] Not sure if the not in is necessary here, makes it a little hard to parse:

Suggested change
document based on whether a field value is in or not in a specified list
of values. Specify the list to the ``in`` option.
document based on whether a field value is in a specified list
of values. Specify the list to the ``in`` option.

that it is reading stale data.

This method takes a ``conditions`` option that allows you to specify
condition to add when {+odm+} checks for uniqueness:

Choose a reason for hiding this comment

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

Suggested change
condition to add when {+odm+} checks for uniqueness:
conditions to add when {+odm+} checks for uniqueness:

Comment on lines 241 to 242
queries a secondary member of the replica set, there is a possibility
that it is reading stale data.

Choose a reason for hiding this comment

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

[s] Suggestion for brevity and to use simple present tense per the style guide:

Suggested change
queries a secondary member of the replica set, there is a possibility
that it is reading stale data.
queries a secondary member of the replica set, it might read stale data.

Comment on lines 276 to 278
Don't use the ``validates_associated`` helper on both ends of your
associations, as this causes {+odm+} to perform validations in an
infinite loop.

Choose a reason for hiding this comment

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

[s] Suggestion to avoid using as per the style guide:

Suggested change
Don't use the ``validates_associated`` helper on both ends of your
associations, as this causes {+odm+} to perform validations in an
infinite loop.
Don't use the ``validates_associated`` helper on both ends of your
associations, because this causes {+odm+} to perform validations in an
infinite loop.

or

Suggested change
Don't use the ``validates_associated`` helper on both ends of your
associations, as this causes {+odm+} to perform validations in an
infinite loop.
Don't use the ``validates_associated`` helper on both ends of your
associations. This causes {+odm+} to perform validations in an
infinite loop.

Trigger Validation
~~~~~~~~~~~~~~~~~~

You can run validations manually by using the ``valid?()`` method.This

Choose a reason for hiding this comment

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

Suggested change
You can run validations manually by using the ``valid?()`` method.This
You can run validations manually by using the ``valid?()`` method. This

:emphasize-lines: 7, 11, 14
:dedent:

{+odm+} behaves differently to Active Record when running ``valid?()``

Choose a reason for hiding this comment

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

[s] For readability, I think "from" is more commonly used and easier to parse:

Suggested change
{+odm+} behaves differently to Active Record when running ``valid?()``
{+odm+} behaves differently from Active Record when running ``valid?()``

{+odm+} behaves differently to Active Record when running ``valid?()``
on persisted data. Active Record's ``valid?()`` runs all
validations, but {+odm+}'s ``valid?()`` runs validations only on
documents that are in memory as an optimization.

Choose a reason for hiding this comment

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

[q] I wasn't sure if this sentence is saying that the way Mongoid runs validations is the optimization, or the documents are in memory as optimization? Maybe add a comma for clarity, or omit:

Suggested change
documents that are in memory as an optimization.
documents that are in memory, as an optimization.
Suggested change
documents that are in memory as an optimization.
documents that are in memory.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

improved wording

Documents </data-modeling/documents>
Field Behaviors </data-modeling/field-behaviors>
Inheritance </data-modeling/inheritance>
Nested Attributes </data-modeling/nested-attributes>
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Decided to make this change after considering what was in that page

@rustagir rustagir requested a review from stephmarie17 December 2, 2024 14:37
Copy link

@stephmarie17 stephmarie17 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 4003671 into mongodb:standardized Dec 2, 2024
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 <[email protected]>

* 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 <[email protected]>

* 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 <[email protected]>

* 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 <[email protected]>
Co-authored-by: Jordan Smith <[email protected]>
Co-authored-by: Dmitry Rybakov <[email protected]>
Co-authored-by: Maya Raman <[email protected]>
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