-
Notifications
You must be signed in to change notification settings - Fork 20
DOCSP-46328: Django connection configuration #142
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-46328: Django connection configuration #142
Conversation
✅ Deploy Preview for docs-pymongo ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
.. tip:: | ||
|
||
To learn how to install the Django ODM and create a | ||
Django project, visit the :ref:`django-get-started` tutorial. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note: this link is broken until #132 is merged
source/configure-connection.txt
Outdated
connection. | ||
|
||
To configure the ``default`` key, include the following | ||
fields: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note to tech reviewers: not sure if "field" is the right word here? are these also keys?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes DATABASES
is a nested dictionary of settings so you might say
To configure the
default
key, include the following nestedkeys
.
Or
To configure the
default
key, include the following settings.
Or
To configure the
default
database, include the following settings.
But you probably wouldn't say "fields".
source/configure-connection.txt
Outdated
@@ -125,7 +125,7 @@ to a MongoDB deployment with the following configuration: | |||
.. tip:: | |||
|
|||
To see a full list of connection options that you | |||
can set in the ``OPTIONS`` field, see the optional | |||
can set in the ``OPTIONS`` nested key, see the optional |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
s/key/dictionary/, otherwise, nice!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
source/configure-connection.txt
Outdated
@@ -51,20 +51,20 @@ which you must configure to set MongoDB as the default database | |||
connection. | |||
|
|||
To configure the ``default`` key, include the following | |||
fields: | |||
nested keys: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To configure the default
database, include the following nested dictionary.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall looks good. I've got two minor changes and that's it!
source/configure-connection.txt
Outdated
- The database you want to use. | ||
|
||
* - **USER** | ||
- The username for authenticating to the database. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can add if your connection requires no auth you do not need to provide it
* - **PASSWORD** | ||
- The password for your database user. | ||
|
||
* - **PORT** |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here you can say the default port is 27017
see the :ref:`django-get-started-connect` step in the Getting Started | ||
tutorial. | ||
|
||
To learn more about Django settings, see `Settings <https://docs.djangoproject.com/en/stable/ref/settings/>`__ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
NIT for future version mappings. We will need to have the links point to the correlating version. Right now it points to stable
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
really nice. a few small wording suggestions and questions
source/configure-connection.txt
Outdated
To manually configure your connection to MongoDB, update | ||
the ``DATABASES`` variable in your project's ``settings.py`` | ||
file. Set the ``DATABASES`` variable to a dictionary value containing | ||
the database settings. You must configure the dictionary's | ||
``default`` key to set MongoDB as the default database | ||
connection. | ||
|
||
To configure the ``default`` key, assign a nested dictionary as its value. | ||
This nested dictionary has the following keys: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
s: i had a hard time understanding this paragraph until i scrolled down to the code example. what do you think about starting with a code example (maybe without any options in it), then going to the table that explains each option, then a fully fleshed out code example?
To manually configure your connection to MongoDB, update | |
the ``DATABASES`` variable in your project's ``settings.py`` | |
file. Set the ``DATABASES`` variable to a dictionary value containing | |
the database settings. You must configure the dictionary's | |
``default`` key to set MongoDB as the default database | |
connection. | |
To configure the ``default`` key, assign a nested dictionary as its value. | |
This nested dictionary has the following keys: | |
To manually configure your connection to MongoDB, update | |
the ``DATABASES`` variable in your project's ``settings.py`` | |
file. Set the ``DATABASES`` variable to a dictionary value containing | |
a key named ``default``, as shown in the following example: | |
.. code-block:: python | |
DATABASES = { | |
"default": { | |
}, | |
} | |
Set the ``default`` key to a dictionary that contains the following keys: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That makes sense, reworked this part!
source/configure-connection.txt
Outdated
| If connecting to a replica set or sharded cluster with multiple hosts, specify | ||
| each host separated by a comma. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
s: slight wording suggestion. could also be helpful to include an example, either here or later.
| If connecting to a replica set or sharded cluster with multiple hosts, specify | |
| each host separated by a comma. | |
| To specify more than one host, include all hostnames in one string. Use | |
| a comma to separate each hostname. |
source/configure-connection.txt
Outdated
This example specifies the ``DATABASES`` variable to connect | ||
to a MongoDB deployment with the following configuration: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
s:
This example specifies the ``DATABASES`` variable to connect | |
to a MongoDB deployment with the following configuration: | |
In this example, the ``DATABASES`` variable specifies the | |
following connection configuration: |
source/configure-connection.txt
Outdated
This example specifies the ``DATABASES`` variable to connect | ||
to a MongoDB deployment with the following configuration: | ||
|
||
- Connects to the ``my_database`` database |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
q: related to the preceding paragraph: who is the actor in the list? the client, DATABASES
, the
connection, the deployment, the app, or something else? It seems to mix client/app behavior ('Connects to the database' with config behavior ('Sets the connection option...')
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was thinking DATABASES. I'll change some of the wording
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wow, English discussion going on here. I would say s/variable to connect to/variable used to connect to/
and reserve further English language usage comments. 😄
source/configure-connection.txt
Outdated
}, | ||
} | ||
|
||
.. tip:: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
s: move this to the OPTIONS row above (and take out of tip box to avoid nested admonition)
Automatically Configure Database Settings | ||
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ | ||
|
||
To automatically construct the ``DATABASES`` setting that configures |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
q: does this literally construct the DATABASES variable in that file with that same structure? or does it just have the same effect on the client/connection?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The Django MongoDB README says it "constructs a DATABASES setting equivalent to the first example" -- @Jibola is it accurate to say that parse_uri() automatically constructs this setting?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
parse_uri
is a convenience method so if it helps to leave it out for clarity's sake then I would do that. That said, DATABASES
has to exist and it has to have the required keys. For example, I can run Django without that setting but if I try to access the admin I get
settings.DATABASES is improperly configured. Please supply the ENGINE value.
Check settings documentation for more details.
Also FWIW my preference for testing is currently: https://github.com/aclark4life/project-templates/blob/main/project_template/project_name/settings/__init__.py#L7 with MONGODB_URI
set by direnv.
And finally, please note we currently use parse_uri
in the project template.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems accurate to say it's constructing the DATABASES setting, so I'll leave that in.
Since I'm showing how to configure the same settings as the previous example, I'll leave the MONGODB_URI variable that shows how to do that
source/configure-connection.txt
Outdated
The following example uses the ``parse_uri()`` method to connect | ||
to a MongoDB deployment with the same configuration as |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
s:
The following example uses the ``parse_uri()`` method to connect | |
to a MongoDB deployment with the same configuration as | |
The following example uses the ``parse_uri()`` method to specify | |
the same connection configuration as |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
NIT: s/method/function/
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm w/ 1 wording change. let me know if you want to brainstorm ideas
source/configure-connection.txt
Outdated
In this example, the ``DATABASES`` variable specifies the | ||
following connection configuration: | ||
|
||
- Connects to the ``my_database`` database | ||
- Sets the database to ``my_database`` | ||
- Provides authentication information for a database user | ||
whose username is ``my_user`` and password is ``my_password`` | ||
- Uses the default MongoDB port (``27017``) | ||
- Specifies the default MongoDB port (``27017``) | ||
- Sets the ``retryWrites`` connection option to ``true``, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i: there's a double verb here--'specifies' in the paragraph and then 'connects'/'sets' in the list items. i can't add a suggestion in GH, but maybe something like this:
In this example, the DATABASES
variable performs the
following steps:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
changed to just "...performs the following actions"
snooty.toml
Outdated
@@ -25,6 +25,7 @@ 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" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can this be "Django MongoDB Backend" or are we required to say "MongoDB Backend for Django" ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can change it! updated
source/configure-connection.txt
Outdated
-------- | ||
|
||
In this guide, you can learn how to connect your Django project to MongoDB | ||
and configure the connection. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FWIW this still reads "funny" to me … if you are connecting your Django project to MongoDB then you've already configured the connection. Feel free to override me because I can be NIT-happy.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reworded
source/configure-connection.txt
Outdated
|
||
.. tip:: | ||
|
||
To learn how to install the Django ODM and create a |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this be {+django-odm+}
instead of "Django ODM" ?
Moved to the Django repo: mongodb/docs-django#6 |
Pull Request Info
PR Reviewing Guidelines
Note: This PR will move to the docs-django repo once it's created
JIRA - https://jira.mongodb.org/browse/DOCSP-46328
Staging - https://deploy-preview-142--docs-pymongo.netlify.app/configure-connection/
Self-Review Checklist