Skip to content

DOCSP-46325: Models #9

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

Merged
merged 13 commits into from
Jan 28, 2025
Merged

Conversation

norareidy
Copy link
Collaborator

@norareidy norareidy commented Jan 17, 2025

Pull Request Info

PR Reviewing Guidelines

JIRA - https://jira.mongodb.org/browse/DOCSP-46325

Staging Links

  • model-data/models
  • model-data
  • 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 Jan 17, 2025

    Deploy Preview for docs-django ready!

    Name Link
    🔨 Latest commit d4a6ece
    🔍 Latest deploy log https://app.netlify.com/sites/docs-django/deploys/6798f84e9461f000083601e8
    😎 Deploy Preview https://deploy-preview-9--docs-django.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.

    Comment on lines 236 to 240
    class Meta:
    # Include metadata here

    def __str__(self):
    # Include logic for displaying your model as a string here
    Copy link
    Collaborator

    Choose a reason for hiding this comment

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

    These two need indentation.

    Copy link
    Collaborator

    @Jibola Jibola left a comment

    Choose a reason for hiding this comment

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

    Saw you refreshed! I'll resolve the ones that have already been fixed. :)

    with {+django-odm+}, see the :ref:`django-models-embedded` section in this guide.

    * - ``ObjectId``
    - ``ObjectIdAutoField``
    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
    - ``ObjectIdAutoField``
    - ``ObjectIdField``

    Copy link
    Collaborator

    @Jibola Jibola left a comment

    Choose a reason for hiding this comment

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

    Save for my two comments with quick resolutions. Everything looks great.

    Copy link
    Collaborator

    @rachel-mack rachel-mack left a comment

    Choose a reason for hiding this comment

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

    I suggestions and questions:

    Copy link
    Collaborator

    Choose a reason for hiding this comment

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

    I'm assuming there are more pages to come that will be nested here?

    Copy link
    Collaborator Author

    Choose a reason for hiding this comment

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

    yep, the indexes page will also go here

    - | Stores an IPv4 or IPv6 address in string format.

    * - ``IntegerField``
    - | Stores integer values.
    Copy link
    Collaborator

    Choose a reason for hiding this comment

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

    Is there a size limit on this? I'm guessing 32 bits?

    Copy link
    Collaborator Author

    @norareidy norareidy Jan 23, 2025

    Choose a reason for hiding this comment

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

    Yes 32 bits, I'll add that

    Use Advanced Fields
    -------------------

    This section how to use the following field types
    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
    This section how to use the following field types
    This section describes how to use the following field types

    Copy link
    Collaborator Author

    Choose a reason for hiding this comment

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

    oops fixed

    However, there is no way to distinguish between the SQL ``NULL`` and the JSON
    ``null`` when querying.

    - Some queries that use ``Q`` objects might not return the expected results.
    Copy link
    Collaborator

    Choose a reason for hiding this comment

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

    Is there a link or something we can provide that explains why?

    Copy link
    Collaborator Author

    Choose a reason for hiding this comment

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

    The only place with more details is the skipped tests in the source code, but I'm not sure if that's useful to link to. I added a little more info (that this is mainly the case with the exclude() method)

    MongoDB ``Object``. To learn more about this field, see the
    :ref:`django-models-embedded` section of this guide.

    {+django-odm+}'s support for ``JSONField`` has the following limitations:
    Copy link
    Collaborator

    Choose a reason for hiding this comment

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

    Suggestion: I think this should be in a Limitations section that follows the Example (at the same level as the example).

    Comment on lines +366 to +375
    - ``base_field``: Specifies the underlying data type of each value
    stored in the array. You cannot specify ``EmbeddedModelField`` or
    ``FileField`` as the base field type.

    - ``size``: Specifies the maximum size of the array. This field is
    optional.

    - ``options``: Specifies Django field options. To view a list of
    available options, see `Field options <{+django-docs+}/ref/models/fields/#field-options>`__
    in the {+framework+} documentation.
    Copy link
    Collaborator

    Choose a reason for hiding this comment

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

    Suggestion: I think you should format these like the Django Fields table above.

    Copy link
    Collaborator Author

    Choose a reason for hiding this comment

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

    since i'm only describing 2-3 arguments in this section + the next one, I think I will leave as is

    Comment on lines +414 to +418
    - ``embedded_model``: Specifies the model class to store.

    - ``options``: Specifies Django field options. To view a list of
    available options, see `Field options <{+django-docs+}/ref/models/fields/#field-options>`__
    in the {+framework+} documentation.
    Copy link
    Collaborator

    Choose a reason for hiding this comment

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

    Suggestion: Format like the Django fields table

    Comment on lines +420 to +424
    .. important::

    The ``makemigrations`` Django command does not detect changes to embedded
    models. If you make changes to the embedded model's class, the model
    stored in the ``EmbeddedModelField`` does not reflect the changes.
    Copy link
    Collaborator

    Choose a reason for hiding this comment

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

    How do you update the embedded model?

    Copy link
    Collaborator Author

    Choose a reason for hiding this comment

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

    There's no way to update the structure of the embedded document, according to the api docs

    Copy link
    Collaborator

    @R-shubham R-shubham left a comment

    Choose a reason for hiding this comment

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

    LGTM

    Define a Model
    --------------

    To create an model that represents a MongoDB collection, add your model
    Copy link
    Collaborator

    Choose a reason for hiding this comment

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

    To create a model

    Copy link
    Collaborator Author

    Choose a reason for hiding this comment

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

    thanks for catching!

    @norareidy norareidy merged commit 5765e26 into mongodb:master Jan 28, 2025
    5 checks passed
    @norareidy norareidy deleted the DOCSP-46325-models branch January 28, 2025 15:34
    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