Opened 13 years ago

Last modified 9 years ago

#16213 closed New feature

View collections in the generic views — at Version 20

Reported by: Harro Owned by: Asif Saifuddin Auvi
Component: Generic views Version: dev
Severity: Normal Keywords: dceu2011
Cc: hvdklauw@…, amirouche.boubekki@…, tom@… Triage Stage: Someday/Maybe
Has patch: yes Needs documentation: yes
Needs tests: yes Patch needs improvement: yes
Easy pickings: no UI/UX: no

Description (last modified by Asif Saifuddin Auvi)

A class that handles multiple class based views and also handles the urls for it.

A base class that just handles the urls for the views.
And for instance a Crud collection does basic crud

"""
Note that REST framework has an existing implementation of view collections...
http://django-rest-framework.org/tutorial/6-viewsets-and-routers.html
http://django-rest-framework.org/api-guide/viewsets.html
http://django-rest-framework.org/api-guide/routers.html
If Django works towards a view collection implementation, probably worth reviewing that approach too.

"""

Change History (22)

comment:1 by Harro, 13 years ago

For good measure, to make this totally awesome #11559 would need to be fixed too.

comment:2 by Harro, 13 years ago

Keywords: dceu2011 added

comment:3 by Harro, 13 years ago

Cc: Harro added

comment:4 by Harro, 13 years ago

Has patch: set
Needs documentation: set
Needs tests: set
Patch needs improvement: set

comment:5 by Harro, 13 years ago

Cc: Harro removed

comment:6 by Harro, 13 years ago

Cc: hvdklauw@… added

comment:7 by Jannis Leidel, 13 years ago

Triage Stage: UnreviewedAccepted

I love it!

comment:8 by Luke Plant, 13 years ago

In the patch, there are various instances of things like "if not self.templates" when what is meant is "if self.templates is not None".

With the queryset attribute, things are more complicated, because you almost certainly don't want to be evaluating a queryset that is stored on a class at any point.

comment:9 by anonymous, 13 years ago

Right.. I'll take that into account

comment:10 by Harro, 13 years ago

Bah, wasn't logged in.

by Harro, 13 years ago

Attachment: 16213.diff added

Added some changes, added a mixin for the crud views to add extra functionality.

by Harro, 13 years ago

Attachment: 16213.2.diff added

Some cleanup

comment:11 by Bas Peschier, 13 years ago

Awesome! but I have a couple of small remarks:

  • Comments in the code are either really short or just "thank you captain obvious"-type, for example the constructor-comments. Remove the unnecessary ones and add some useful docstrings to the main classes to give the reader a good idea what their purpose is.
  • PEP8 nit-picking: Crud "should be" CRUD in the class names to emphasise the acronym.

comment:12 by Harro, 13 years ago

milestone: 1.4

Right you are, didn't focus much on docs(trings) and tests yet, just on getting the skeleton api right. Installed the pep8 checker in sublime after the last changes so that should be fixed next time too.

comment:13 by Jacob, 13 years ago

milestone: 1.4

Milestone 1.4 deleted

comment:14 by Amirouche, 11 years ago

Cc: amirouche.boubekki@… added

comment:15 by Amirouche, 11 years ago

It looks like AdminSite class

  • «ViewCollection» should be outside views.generic , it is not a view and not a generic view, but I don't know where.
  • The name is misleading, based on your description and the code it's more a «ClassBasedViewCollection», but I still believe that «ViewCollection» is a good idea, it should support any callable.
  • Based on the example CRUDViewsCollection, «ViewCollection.current_app» class property is not relevant, it should be configurable per instance (init argument) moreover «current_app» variable nae is used only in reverse which I think is only used to override urlresolver namespace strategy so better use application_namespace/instance_namespace (IMO prefered) or respectively app_name/namespace the keyword arguments of include.
  • I don't like the following code in constructor:
    for key, value in kwargs.iteritems():
        setattr(self, key, value)
    
  • get_urls
    • patterns can take a list of urls
    • The code enforce django CBV type of class, this makes other callables impossible to register
  • urls property
    • This is not a real property, you can not set it
    • It only returns urls, instance_namespace where the include also accepts another argument application_namespace
    • This is not a set of urls or urlpatterns, I better see it be url_include or the like and change the return to django.conf.urls.include(pattern_list, self.instance_namespace, self.application_namespace)
  • get_urls_namespacestring I don't see why this is useful, the view should compute its namespace, the naming is not good and Python avoids «get_»

Related tickets:

comment:16 by Tom Christie, 11 years ago

Cc: tom@… added

comment:17 by Tom Christie, 11 years ago

Note that REST framework has an existing implementation of view collections...

http://django-rest-framework.org/tutorial/6-viewsets-and-routers.html
http://django-rest-framework.org/api-guide/viewsets.html
http://django-rest-framework.org/api-guide/routers.html

If Django works towards a view collection implementation, probably worth reviewing that approach too.

comment:18 by Asif Saifuddin Auvi, 9 years ago

@tomchristie you idea is nice! i would love to have that on django proper!

comment:19 by Asif Saifuddin Auvi, 9 years ago

Owner: changed from Harro to Asif Saifuddin Auvi
Status: newassigned
Version: 1.3master

comment:20 by Asif Saifuddin Auvi, 9 years ago

Description: modified (diff)
Note: See TracTickets for help on using tickets.
Back to Top