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

#18451 Improved class based view documentation #144

Closed
wants to merge 90 commits into
base: master
from

Conversation

Projects
None yet
@pydanny
Contributor

pydanny commented Jun 8, 2012

For https://code.djangoproject.com/ticket/18451

Ref Improvements:

  • Documentation moved to /ref/class-based-views/ directory.
  • Method Resolution Order (MRO) on most views.
  • Method flowchart added to many views.
  • Mixins moved to their own pages.
  • Mixins demonstrate which other Mixins they extend.
  • Autodoc added to all views and mixins.
    • Because the Django code does always not contain docstrings, or it is not complete, there remains in many cases the old manual documentation.
    • 'Note' statements placed whenever autodoc is in addition to manual documentation.

Topic Improvements:

  • Documentation moved to /topics/class-based-views/ directory.
  • Cleaner explanation including additional examples.
  • More examples.
  • Mixins moved to their own pages.
  • Very clean mixin documentation with advocacy for best practices.

Authors:

  • Daniel Greenfeld
  • James Aylett
  • Marc Tamlyn
  • Simon Williams

mjtamlyn and others added some commits Jun 7, 2012

Changed references to 'simple generic class based views' to 'Fundamen…
…tal class based views'. Also worked on some new dialogue to help explain things
Best practices in list and detail views examples.
Try to make the examples more consistent and follow some best practices.
For example: all strings are single quoted, views are always defined via
subclassing, and certain 1.4 now features are given for reference rather
than re-implementing them.
Merge pull request #1 from jaylett/cbv-docs
Cbv docs changes provided by @jaylett
Merge pull request #2 from SystemParadox/cbv-docs
Initial form handling CBV docs
Add some descriptions missing for core mixins.
Add "Methods & Attributes" headers throughout CBV mixin ref docs.
@jezdez

View changes

Show outdated Hide outdated docs/ref/class-based-views/fundamentals.txt
def get_redirect_url(self, pk):
article = get_object_or_404(Article, pk=pk)
article.count += 1

This comment has been minimized.

@jezdez

jezdez Jun 9, 2012

Member

The example is well-intended but maybe should do something else, or use the F object.

@jezdez

jezdez Jun 9, 2012

Member

The example is well-intended but maybe should do something else, or use the F object.

This comment has been minimized.

@mjtamlyn

mjtamlyn Jun 9, 2012

Member

Opted for calling an imaginary update_counter method on the article. Also set it to be non-permanent redirect otherwise it wouldn't be much use as a view (I believe the browser would skip hitting this url at all in future)

@mjtamlyn

mjtamlyn Jun 9, 2012

Member

Opted for calling an imaginary update_counter method on the article. Also set it to be non-permanent redirect otherwise it wouldn't be much use as a view (I believe the browser would skip hitting this url at all in future)

This comment has been minimized.

@jezdez

jezdez Jun 9, 2012

Member

Cool!

@jezdez

jezdez Jun 9, 2012

Member

Cool!

@jezdez

View changes

Show outdated Hide outdated docs/ref/class-based-views/generic-date-based.txt
have objects available according to ``queryset``, represented as
``datetime.datetime`` objects, in ascending order.
* ``year``: A ``datetime.date`` object representing the given year.

This comment has been minimized.

@jezdez

jezdez Jun 9, 2012

Member

We could use the intersphinx ability in Sphinx here to link to the Python docs.

@jezdez

jezdez Jun 9, 2012

Member

We could use the intersphinx ability in Sphinx here to link to the Python docs.

This comment has been minimized.

@mjtamlyn

mjtamlyn Jun 9, 2012

Member

Would be nice. Couldn't seem to get it to work.

It looks like it's been set up to link to the python docs in the conf.py but :ref:datetime.date<python:anything-here> (with the necessary back ticks) was giving me an error:

WARNING: undefined label: python:datetime (if the link has no caption the label must precede a section header)

@mjtamlyn

mjtamlyn Jun 9, 2012

Member

Would be nice. Couldn't seem to get it to work.

It looks like it's been set up to link to the python docs in the conf.py but :ref:datetime.date<python:anything-here> (with the necessary back ticks) was giving me an error:

WARNING: undefined label: python:datetime (if the link has no caption the label must precede a section header)

This comment has been minimized.

@jezdez

jezdez Jun 9, 2012

Member

Try :class:datetime.date

@jezdez

jezdez Jun 9, 2012

Member

Try :class:datetime.date

@jezdez

View changes

Show outdated Hide outdated docs/ref/class-based-views/generic-date-based.txt
ArchiveIndexView
~~~~~~~~~~~~~~~~
.. class:: ArchiveIndexView()

This comment has been minimized.

@jezdez

jezdez Jun 9, 2012

Member

I don't think brackets are needed here for the class directive.

@jezdez

jezdez Jun 9, 2012

Member

I don't think brackets are needed here for the class directive.

@jezdez

View changes

Show outdated Hide outdated docs/ref/class-based-views/generic-date-based.txt
:class:`~django.views.generic.dates.BaseDateListView`), the template's
context will be:
* ``date_list``: A ``DateQuerySet`` object containing all days that

This comment has been minimized.

@jezdez

jezdez Jun 9, 2012

Member

Could we link to DateQuerySet here?

@jezdez

jezdez Jun 9, 2012

Member

Could we link to DateQuerySet here?

@jezdez

View changes

Show outdated Hide outdated docs/ref/class-based-views/generic-display.txt
def get_context_data(self, **kwargs):
context = super(ArticleDetailView, self).get_context_data(**kwargs)
context['now'] = datetime.now()

This comment has been minimized.

@jezdez

jezdez Jun 9, 2012

Member

We should discourage using datetime.now given the new timezone abilities in 1.4. Instead use django.utils.timezone.now.

@jezdez

jezdez Jun 9, 2012

Member

We should discourage using datetime.now given the new timezone abilities in 1.4. Instead use django.utils.timezone.now.

@jezdez

View changes

Show outdated Hide outdated docs/ref/class-based-views/index.txt
:meth:`~View.as_view()` classmethod::
urlpatterns = patterns('',
(r'^view/$', MyView.as_view(size=42)),

This comment has been minimized.

@jezdez

jezdez Jun 9, 2012

Member

Indented wrong.

@jezdez

jezdez Jun 9, 2012

Member

Indented wrong.

@jezdez

View changes

Show outdated Hide outdated docs/ref/class-based-views/index.txt
Any argument passed into :meth:`~View.as_view()` will be assigned onto the
instance that is used to service a request. Using the previous example,
this means that every request on ``MyView`` is able to interrogate

This comment has been minimized.

@jezdez

jezdez Jun 9, 2012

Member

"use" instead of "interrogate"?

@jezdez

jezdez Jun 9, 2012

Member

"use" instead of "interrogate"?

@jezdez

View changes

Show outdated Hide outdated docs/ref/class-based-views/index.txt
-----------------------------------------
Fundamental class-based views can be thought of as *base* views, which can be
used by themselves or inherited from. They may not provide all the

This comment has been minimized.

@jezdez

jezdez Jun 9, 2012

Member

Double space after dot.

@jezdez

jezdez Jun 9, 2012

Member

Double space after dot.

@jezdez

View changes

Show outdated Hide outdated docs/ref/class-based-views/index.txt
Fundamental class-based views can be thought of as *base* views, which can be
used by themselves or inherited from. They may not provide all the
capabilities required for projects, in which case there are Mixins which extend
what Fundamental views can do.

This comment has been minimized.

@jezdez

jezdez Jun 9, 2012

Member

lower case for "Fundamental"?

@jezdez

jezdez Jun 9, 2012

Member

lower case for "Fundamental"?

@jezdez

View changes

Show outdated Hide outdated docs/ref/class-based-views/mixins-editing.txt
Retrieve initial data for the form. By default, returns a copy of
:attr:`~django.views.generic.edit.FormMixin.initial`.
.. admonition:: Changed in 1.4

This comment has been minimized.

@jezdez

jezdez Jun 9, 2012

Member

I think this can also be expressed with a .. versionchanged:: 1.4 directive.

@jezdez

jezdez Jun 9, 2012

Member

I think this can also be expressed with a .. versionchanged:: 1.4 directive.

@jezdez

View changes

Show outdated Hide outdated docs/ref/class-based-views/mixins-editing.txt
.. note::
This is named 'ProcessFormView' and inherits directly from :class:`django.views.generic.base.View`,
but breaks if used independently. Therefore is actually a mixin.

This comment has been minimized.

@jezdez

jezdez Jun 9, 2012

Member

Typo in last sentence.

@jezdez

jezdez Jun 9, 2012

Member

Typo in last sentence.

@jezdez

View changes

Show outdated Hide outdated docs/ref/class-based-views/mixins-single-object.txt
The field on the current object instance that can be used to determine
the name of a candidate template. If either ``template_name_field`` or
the value of the ``template_name_field`` on the current object instance
is ``None``, the object will not be interrogated for a candidate

This comment has been minimized.

@jezdez

jezdez Jun 9, 2012

Member

"interrogated"?

@jezdez

jezdez Jun 9, 2012

Member

"interrogated"?

@jezdez

View changes

Show outdated Hide outdated docs/topics/class-based-views/generic-display.txt
comes from the name of the app that defines the model, while the "publisher"
bit is just the lowercased version of the model's name.
.. note:: Thus, when (for example) the :class:`django.template.loaders.app_directories.Loader` template loader is enabled in :setting:`TEMPLATE_LOADERS`, a template location could be: /path/to/project/books/templates/books/publisher_list.html

This comment has been minimized.

@jezdez

jezdez Jun 9, 2012

Member

Please wrap the lines of this note.

@jezdez

jezdez Jun 9, 2012

Member

Please wrap the lines of this note.

@jezdez

View changes

Show outdated Hide outdated docs/topics/class-based-views/generic-display.txt
.. note::
Generally, get_context_data will merge the context data of all parent classes
with those of the current class. To preserve this behavior in your own classes

This comment has been minimized.

@jezdez

jezdez Jun 9, 2012

Member

Two spaces after the dot.

@jezdez

jezdez Jun 9, 2012

Member

Two spaces after the dot.

from books.views import AuthorDetailView
urlpatterns = patterns('',

This comment has been minimized.

@jezdez

jezdez Jun 9, 2012

Member

PEP8 please.

@jezdez

jezdez Jun 9, 2012

Member

PEP8 please.

@jezdez

View changes

Show outdated Hide outdated docs/topics/class-based-views/generic-editing.txt
.. note::
When specifying a custom form class, you must still specify the model,
even though the :attr:`form_class` may be a :class:`ModelForm`.
(See https://code.djangoproject.com/ticket/15125)

This comment has been minimized.

@jezdez

jezdez Jun 9, 2012

Member

Not sure if linking a ticket is really sensible here.

@jezdez

jezdez Jun 9, 2012

Member

Not sure if linking a ticket is really sensible here.

@jezdez

View changes

Show outdated Hide outdated docs/topics/class-based-views/generic-editing.txt
success_url = reverse_lazy('author-list')
.. note::
We have to use ``reverse_lazy`` here, not just ``reverse`` as the urls are

This comment has been minimized.

@jezdez

jezdez Jun 9, 2012

Member

Link to the reverse_lazy docs here?

@jezdez

jezdez Jun 9, 2012

Member

Link to the reverse_lazy docs here?

@jezdez

View changes

Show outdated Hide outdated docs/topics/class-based-views/index.txt
More than just HTML
-------------------
Where class based view shine is when you want to do the same thing many times.

This comment has been minimized.

@jezdez

jezdez Jun 9, 2012

Member

s/view/views/g

@jezdez

jezdez Jun 9, 2012

Member

s/view/views/g

@jezdez

View changes

Show outdated Hide outdated docs/topics/class-based-views/mixins.txt
interface to working with templates in class-based views.
:class:`~django.views.generic.base.TemplateResponseMixin`
Everywhere that needs to make a

This comment has been minimized.

@jezdez

jezdez Jun 9, 2012

Member

"Everywhere that needs to make.." sounds odd to me.

@jezdez

jezdez Jun 9, 2012

Member

"Everywhere that needs to make.." sounds odd to me.

@jezdez

View changes

Show outdated Hide outdated docs/topics/class-based-views/mixins.txt
Everywhere that needs to make a
:class:`~django.template.response.TemplateResponse` will call the
:meth:`~django.views.generic.base.TemplateResponseMixin.render_to_response`
method that :class:`TemplateResponseMixin` provides. Most of the time

This comment has been minimized.

@jezdez

jezdez Jun 9, 2012

Member

Double space.

@jezdez

jezdez Jun 9, 2012

Member

Double space.

@jezdez

View changes

Show outdated Hide outdated docs/topics/class-based-views/mixins.txt
.. versionadded:: 1.5
:class:`~django.views.generic.base.ContextMixin`
Everywhere that needs context data, such as for rendering a

This comment has been minimized.

@jezdez

jezdez Jun 9, 2012

Member

"Everywhere that needs context data" sounds odd.

@jezdez

jezdez Jun 9, 2012

Member

"Everywhere that needs context data" sounds odd.

self.object = self.get_object()
# Actually record interest somehow here!
return HttpResponseRedirect(

This comment has been minimized.

@jezdez

jezdez Jun 9, 2012

Member

PEP8

@jezdez

jezdez Jun 9, 2012

Member

PEP8

@jezdez

View changes

Show outdated Hide outdated docs/topics/class-based-views/mixins.txt
urlpatterns = patterns('',
#...
**url(

This comment has been minimized.

@jezdez

jezdez Jun 9, 2012

Member

PEP8

@jezdez

jezdez Jun 9, 2012

Member

PEP8

@jezdez

This comment has been minimized.

Show comment
Hide comment
@jezdez

jezdez Jun 9, 2012

Member

Except some optional intersphinx links this looks good to me. If there are no other reviews, I'd commit this tomorrow.

Member

jezdez commented Jun 9, 2012

Except some optional intersphinx links this looks good to me. If there are no other reviews, I'd commit this tomorrow.

@mjtamlyn

This comment has been minimized.

Show comment
Hide comment
@mjtamlyn

mjtamlyn Jun 10, 2012

Member

@jezdez, master of intersphinx. I believe all outstanding blocking issues are resolved.

Member

mjtamlyn commented Jun 10, 2012

@jezdez, master of intersphinx. I believe all outstanding blocking issues are resolved.

@jezdez

This comment has been minimized.

Show comment
Hide comment
@jezdez

jezdez Jun 10, 2012

Member

Those changes fixed the build for me: https://gist.github.com/2907251

Member

jezdez commented Jun 10, 2012

Those changes fixed the build for me: https://gist.github.com/2907251

@jezdez

This comment has been minimized.

Show comment
Hide comment
@jezdez

jezdez Jun 10, 2012

Member

Oh, I think I still have two issues:

  • The word "Fundamentals" for describing the base class based views seems a bit off to me. It may be my non-nativeness but it sounds like "Fundamentalists" which has a rather negative connotation. I know "base" is also pretty loaded, so what if we call it "Basic class based views". Alternatively "Essential class based views"?
  • The method flowcharts need some nice graphics, or at least links to the appropriate methods instead of just a list of text.

Given those two important things are still open, I won't merge this just yet. Any help with solving would be much appreciated.

Member

jezdez commented Jun 10, 2012

Oh, I think I still have two issues:

  • The word "Fundamentals" for describing the base class based views seems a bit off to me. It may be my non-nativeness but it sounds like "Fundamentalists" which has a rather negative connotation. I know "base" is also pretty loaded, so what if we call it "Basic class based views". Alternatively "Essential class based views"?
  • The method flowcharts need some nice graphics, or at least links to the appropriate methods instead of just a list of text.

Given those two important things are still open, I won't merge this just yet. Any help with solving would be much appreciated.

@mjtamlyn

This comment has been minimized.

Show comment
Hide comment
@mjtamlyn

mjtamlyn Jun 10, 2012

Member

Hmmm. I like fundamental. To me there's no connection with fundamentalist, and we're using it in the meaning of "Foundational" (which is a worse word) - kinda meaning that they're the important ones and the ones upon which everything else is built. Basic has connotations of simple, and I'm not sure they are... Essential also implies you have to use them, which you don't.

Flowcharts are a bit more of a question. I probably need to do a good review of them to make sure they're actually accurate. Some graphviz graphics would be nice, but I'm concerned about them not being updated properly. Either we build them first and commit the images to the repo, or we use http://sphinx.pocoo.org/ext/graphviz.html to build them dynamically. This would require everything to have the relevant binaries installed though. Opinions @jacobian?

A lot of the ref docs are incomplete - which includes the method flows. Not every method is documented yet (see previous discussion about includes/autodoc) - I've not written all of the replacement references now the autobuilt stuff is gone. Do we want to get it all accurate and finished before we merge anything?

Re those build changes, I'm not getting errors but that is tidier rst so I'll make those tweaks.

Member

mjtamlyn commented Jun 10, 2012

Hmmm. I like fundamental. To me there's no connection with fundamentalist, and we're using it in the meaning of "Foundational" (which is a worse word) - kinda meaning that they're the important ones and the ones upon which everything else is built. Basic has connotations of simple, and I'm not sure they are... Essential also implies you have to use them, which you don't.

Flowcharts are a bit more of a question. I probably need to do a good review of them to make sure they're actually accurate. Some graphviz graphics would be nice, but I'm concerned about them not being updated properly. Either we build them first and commit the images to the repo, or we use http://sphinx.pocoo.org/ext/graphviz.html to build them dynamically. This would require everything to have the relevant binaries installed though. Opinions @jacobian?

A lot of the ref docs are incomplete - which includes the method flows. Not every method is documented yet (see previous discussion about includes/autodoc) - I've not written all of the replacement references now the autobuilt stuff is gone. Do we want to get it all accurate and finished before we merge anything?

Re those build changes, I'm not getting errors but that is tidier rst so I'll make those tweaks.

@pydanny

This comment has been minimized.

Show comment
Hide comment
@pydanny

pydanny Jun 11, 2012

Contributor

@idangazit I'm going to find you on IRC. Right now.

Contributor

pydanny commented Jun 11, 2012

@idangazit I'm going to find you on IRC. Right now.

@pydanny pydanny closed this Jun 11, 2012

@pydanny pydanny reopened this Jun 11, 2012

@jezdez

This comment has been minimized.

Show comment
Hide comment
@jezdez

jezdez Jun 11, 2012

Member

I'm going to close this as I think it's ready for commit (I have all the changes locally). I've decided to not go with fundamental as I think it would introduce more confusion. But that's really a minor thing and I'm happy to say this is going to be a HUGE help for our users. Thank you good sirs.

Member

jezdez commented Jun 11, 2012

I'm going to close this as I think it's ready for commit (I have all the changes locally). I've decided to not go with fundamental as I think it would introduce more confusion. But that's really a minor thing and I'm happy to say this is going to be a HUGE help for our users. Thank you good sirs.

@jezdez jezdez closed this Jun 11, 2012

@idan

This comment has been minimized.

Show comment
Hide comment
@idan

idan Jun 11, 2012

Member

🍰

Member

idan commented Jun 11, 2012

🍰

@bryanhelmig

This comment has been minimized.

Show comment
Hide comment
@bryanhelmig

bryanhelmig Jun 12, 2012

Contributor

Just in time to make us look foolish (http://3rdaverad.io/shows/django-podcast/episodes/class-vs-function-based-views/) as we complain about lack of documentation... :-D

Contributor

bryanhelmig commented Jun 12, 2012

Just in time to make us look foolish (http://3rdaverad.io/shows/django-podcast/episodes/class-vs-function-based-views/) as we complain about lack of documentation... :-D

sztrovacsek pushed a commit to sztrovacsek/django that referenced this pull request Mar 7, 2015

nanuxbe pushed a commit to nanuxbe/django that referenced this pull request Jul 2, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment