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-43200: Get started with Django #132

Open
wants to merge 34 commits into
base: django-mongodb-backend
Choose a base branch
from

Conversation

norareidy
Copy link
Collaborator

@norareidy norareidy commented Nov 26, 2024

Pull Request Info

PR Reviewing Guidelines

JIRA - https://jira.mongodb.org/browse/DOCSP-43200
Staging - https://deploy-preview-132--docs-pymongo.netlify.app/django-get-started/

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 26, 2024

Deploy Preview for docs-pymongo ready!

Name Link
🔨 Latest commit ea02256
🔍 Latest deploy log https://app.netlify.com/sites/docs-pymongo/deploys/678926df4acf960008c906b6
😎 Deploy Preview https://deploy-preview-132--docs-pymongo.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

@aclark4life aclark4life left a comment

Choose a reason for hiding this comment

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

LGTM pending various small changes, thanks @norareidy

@@ -61,7 +61,7 @@ in your development environment.

.. code-block:: bash

pip install git+https://github.com/mongodb-labs/django-mongodb
pip install git+https://github.com/mongodb-labs/django-mongodb-backend

Choose a reason for hiding this comment

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

The alpha release is now on PyPI, so maybe this should be pip install django-mongodb-backend. 🤔


pip install git+https://github.com/mongodb-labs/django-mongodb-backend

This command also installs the following dependencies:

Choose a reason for hiding this comment

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

Also those dependencies' dependencies but quite possibly not worth mentioning.

  • asgiref
  • dnspython
  • sqlparse
django-mongodb-backend v5.0a1
├── django v5.0.10
│   ├── asgiref v3.8.1
│   └── sqlparse v0.5.3
├── pymongo v4.10.1
│   └── dnspython v2.7.0

snooty.toml Outdated Show resolved Hide resolved
:values: tutorial

.. meta::
:description: Learn how to create an app to connect to a MongoDB deployment by using Django MongoDB.

Choose a reason for hiding this comment

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

s/Django MongoDB/Django MongoDB Backend/

source/index.txt Outdated
@@ -13,6 +13,7 @@ MongoDB {+driver-short+} Documentation
.. toctree::

Get Started </get-started>
Get Started with Django MongoDB </django-get-started>

Choose a reason for hiding this comment

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

s/Django MongoDB/Django MongoDB Backend/

@norareidy norareidy changed the base branch from master to django-mongodb-backend January 8, 2025 22:00
Copy link

@aclark4life aclark4life left a comment

Choose a reason for hiding this comment

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

LGTM, thanks

@rustagir rustagir self-requested a review January 10, 2025 19:04
.. code-block:: python

from django.db import models
from django_mongodb_backend.fields import EmbeddedModelField, ArrayField
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Note: the EmbeddedModelField import and the awards Movie field below will cause errors until this PR is merged

Copy link

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

LGTM!!

text = models.CharField(max_length=100)

class Meta:
abstract = True

Choose a reason for hiding this comment

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

This should be omitted. Abstract models can't be instantiated so this approach isn't compatible with the current implementation. There isn't currently a way to avoid the collection from being created separately. I'm still thinking about how we could solve this.


You can create a Django admin site to edit your application's
data from a web interface. To learn more about the Django admin
site and its features, see `The Django admin site <https://docs.djangoproject.com/en/5.1/ref/contrib/admin/>`__

Choose a reason for hiding this comment

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

There's a mismatch between the version number in the Django links and "django-version-number".

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Updated to use the django-version-number constant


class Movie(models.Model):
title = models.CharField(max_length=200)
plot = models.TextField(null=True)

Choose a reason for hiding this comment

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

"Avoid using null on string-based fields such as CharField and TextField. The Django convention is to use an empty string, not NULL, as the “no data” state for string-based fields. If a string-based field has null=False, empty strings can still be saved for “no data”. If a string-based field has null=True, that means it has two possible values for “no data”: NULL, and the empty string. In most cases, it’s redundant to have two possible values for “no data”.

.. code-block:: bash

DATABASES = {
"default": {

Choose a reason for hiding this comment

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

The indentation is incorrect throughout these documents (only 3 spaces).

title = models.CharField(max_length=200)
plot = models.TextField(null=True)
runtime = models.IntegerField(default=0)
released = models.DateTimeField("release date", null=True)

Choose a reason for hiding this comment

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

Typically, any time you have null=True, you'll also need blank=True.

Comment on lines 27 to 28
python3 manage.py shell
from sample_mflix.models import Movie, Viewer

Choose a reason for hiding this comment

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

This is a combination of bash and pycon.

Before installing {+django-odm+}, ensure you have the following dependencies installed
in your development environment:

- `Python3 version 3.10 or later <https://www.python.org/downloads/>`__

Choose a reason for hiding this comment

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

"Python 3.10 or later"

in your development environment:

- `Python3 version 3.10 or later <https://www.python.org/downloads/>`__
- `pip <https://pip.pypa.io/en/stable/installation/>`__

Choose a reason for hiding this comment

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

Python 3 includes pip, so I think there should be no need install it separately.


def index(request):
return HttpResponse("Hello, world. You're at the application index.")

Choose a reason for hiding this comment

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

Extra blank line compared to rest of example.


.. code-block:: bash

python3 manage.py startapp sample_mflix --template https://github.com/mongodb-labs/django-mongodb-app/archive/refs/heads/5.0.x.zip

Choose a reason for hiding this comment

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

use {+django-version-number+} as in startproject?

@norareidy norareidy requested a review from timgraham January 13, 2025 20:16
Atlas database user's username and password. Then, replace the
``<connection string URI>`` placeholder with the connection string
that you copied from the :ref:`django-get-started-connection-string`
step of this guide.
Copy link

@R-shubham R-shubham Jan 13, 2025

Choose a reason for hiding this comment

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

@norareidy The connection string that user copies will have username and password by default. If they are using this approach then "HOST" will expect connection string in this format: "mongodb+srv://cluster0.example.mongodb.net". @Jibola / @aclark4life can you confirm?

Copy link

@aclark4life aclark4life Jan 13, 2025

Choose a reason for hiding this comment

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

There are at least two things going on here:

  • DATABASES dictionary: DATABASES["default"]["HOST"] can accept a prefix of mongodb+srv:// but does not require one.
  • django_mongodb_backend.parse_uri: Connection string is terminology only used in the context of parse_uri and in that context, the MONGODB_URI from Atlas can be "cut and pasted" with the exception of the NAME key which is not present in the Atlas MONGODB_URI but must be present in the parse_uri connection string.

To put it another way:

  • DATABASES dictionary: DATABASES["default"]["NAME"] must be defined. -or-
  • django_mongodb_backend.parse_uri: MONGODB_URI must include /mydatabase e.g. mongodb+srv://cluster0.example.mongodb.net/mydatabase.

Again, if it's easier to steer clear of parse_uri for clarity's sake that's OK.

snooty.toml Outdated
@@ -25,12 +25,14 @@ sharedinclude_root = "https://raw.githubusercontent.com/10gen/docs-shared/main/"
driver-short = "PyMongo"
driver-long = "PyMongo, the MongoDB synchronous Python driver,"
driver-async = "PyMongo Async"
django-odm = "Django MongoDB"

Choose a reason for hiding this comment

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

s/Django MongoDB/Django MongoDB Backend/?

@@ -25,12 +26,15 @@ sharedinclude_root = "https://raw.githubusercontent.com/10gen/docs-shared/main/"
driver-short = "PyMongo"
driver-long = "PyMongo, the MongoDB synchronous Python driver,"
driver-async = "PyMongo Async"
django-odm = "MongoDB Backend for Django"
django-api = "https://django-mongodb.readthedocs.io/en/latest/"

Choose a reason for hiding this comment

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

Be aware this is changing to "django-mongodb-backend".

Create an Admin Site </django-get-started/django-create-admin/>
Next Steps </django-get-started/django-next-steps/>

{+django-odm+} is a PyMongo integration that allows you to use the

Choose a reason for hiding this comment

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

Open to what others think, but I would call it "a MongoDB integration that uses PyMongo under the hood." i.e. the primary purpose is to use MongoDB! The fact that it uses PyMongo is incidental.

Choose a reason for hiding this comment

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

Agree. "Django MongoDB Backend is a MongoDB database backend for Django that uses PyMongo to connect to MongoDB and implements the Django database specification so users can use MongoDB with Django."

I tend to avoid colloquialisms like "under the hood" and PyMongo is incidental here.

Choose a reason for hiding this comment

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

That is a rather verbose and redundant sentence. 😆

"Django MongoDB Backend is a Django database backend that uses PyMongo to connect to MongoDB." might be sufficient!

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Updated! Do you have a preference between "Django MongoDB Backend" and "MongoDB Backend for Django"?

Choose a reason for hiding this comment

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

The other backends I've written use the latter phrasing, however, since we had to rename our package from django-mongondb to django-mongodb-backend, we can use the former.


.. code-block:: bash

python3 manage.py runserver

Choose a reason for hiding this comment

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

Maybe this is consistent with the rest of your documentation and you want to leave it alone, but if you are working in a Python 3 virtual environment, you can write python without the 3. Coming from Django's docs, it looks odd to include that 3 on all commands.

Choose a reason for hiding this comment

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

I was going to raise this too, but would defer to MongoDB-wide conventions. If there are no such conventions, then I prefer python too.

Open your ``settings.py`` file and navigate to the ``DATABASES`` setting.
Replace this setting with the following code:

.. code-block:: bash

Choose a reason for hiding this comment

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

python


After completing these steps, you have a connection string that
contains your database username and password.

Choose a reason for hiding this comment

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

You may want to delete the stray blank line(s) at the end of some documents. ;-)

.. code-block:: bash

python3 manage.py makemigrations sample_mflix
python3 manage.py migrate

Choose a reason for hiding this comment

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

sample_mflix is an existing database, so the models should have an inner Meta class with managed = False so that Django doesn't try to create (and otherwise modify) the collections. Otherwise, I believe MongoDB will error with "collection already exists) here when running migrate...

Choose a reason for hiding this comment

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

@timgraham As well as set db_name correct?

Choose a reason for hiding this comment

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

Yes, db_table is already set.

This command also installs the following dependencies:

- PyMongo version {+version-number+} and its dependencies
- Django version {+django-version-number+} and its dependencies

Choose a reason for hiding this comment

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

More precisely, this will be "The latest Django 5.0.x version...'


This code returns the name of the matching user:

.. code-block:: bash

Choose a reason for hiding this comment

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

Technically the formatter is "pycon" but since you don't format your console sessions normally, I'm not sure it matters.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

pycon is not an option for the .. code-block:: directive unfortunately

movie_awards = Award(wins=122, nominations=245, text="Won 1 Oscar")
movie = Movie(title="Minari", plot=
"A Korean-American family moves to an Arkansas farm in search of their own American Dream",
runtime=217, released=timezone.make_aware(datetime(2020, 1, 26, 0, 0)),

Choose a reason for hiding this comment

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

0, 0 is optional (default values)

.. code-block:: python

movie_awards = Award(wins=122, nominations=245, text="Won 1 Oscar")
movie = Movie(title="Minari", plot=

Choose a reason for hiding this comment

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

If you like, it would be more readable to format these using hanging indent, e.g.

movie = Movie(
    title="Minari",
    plot="...",
    ...
)

Also, you can use movie = Movie.objects.create(...)if you want to avoid the separate save() step. That's the usual pattern unless you need to do something between initializing the model and saving it.

@timgraham timgraham mentioned this pull request Jan 14, 2025
5 tasks
Copy link

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

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.

6 participants