Opened 5 years ago

Closed 19 months ago

Last modified 19 months ago

#16213 closed New feature (fixed)

View collections in the generic views

Reported by: Harro Owned by: Asif Saifuddin Auvi
Component: Generic views Version: master
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.

"""

Attachments (2)

16213.diff (6.8 KB) - added by Harro 5 years ago.
Added some changes, added a mixin for the crud views to add extra functionality.
16213.2.diff (6.9 KB) - added by Harro 5 years ago.
Some cleanup

Download all attachments as: .zip

Change History (24)

comment:1 Changed 5 years ago by Harro

Needs documentation: unset
Needs tests: unset
Patch needs improvement: unset

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

comment:2 Changed 5 years ago by Harro

Keywords: dceu2011 added

comment:3 Changed 5 years ago by Harro

Cc: Harro added

comment:4 Changed 5 years ago by Harro

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

comment:5 Changed 5 years ago by Harro

Cc: Harro removed

comment:6 Changed 5 years ago by Harro

Cc: hvdklauw@… added

comment:7 Changed 5 years ago by Jannis Leidel

Triage Stage: UnreviewedAccepted

I love it!

comment:8 Changed 5 years ago by Luke Plant

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 Changed 5 years ago by anonymous

Right.. I'll take that into account

comment:10 Changed 5 years ago by Harro

Bah, wasn't logged in.

Changed 5 years ago by Harro

Attachment: 16213.diff added

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

Changed 5 years ago by Harro

Attachment: 16213.2.diff added

Some cleanup

comment:11 Changed 5 years ago by Bas Peschier

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 Changed 5 years ago by Harro

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 Changed 5 years ago by Jacob

milestone: 1.4

Milestone 1.4 deleted

comment:14 Changed 4 years ago by Amirouche

Cc: amirouche.boubekki@… added

comment:15 Changed 4 years ago by Amirouche

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 Changed 3 years ago by Tom Christie

Cc: tom@… added

comment:17 Changed 3 years ago by Tom Christie

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 Changed 19 months ago by Asif Saifuddin Auvi

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

comment:19 Changed 19 months ago by Asif Saifuddin Auvi

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

comment:20 Changed 19 months ago by Asif Saifuddin Auvi

Description: modified (diff)

comment:21 Changed 19 months ago by Tom Christie

Resolution: fixed
Status: assignedclosed
Triage Stage: AcceptedSomeday/Maybe

I'm going to suggest that this isn't realistically in-scope for being worked on at this point.
It could be picked up in core at some point, but doesn't feel likely right now.

comment:22 in reply to:  21 Changed 19 months ago by Asif Saifuddin Auvi

Replying to tomchristie:

I'm going to suggest that this isn't realistically in-scope for being worked on at this point.
It could be picked up in core at some point, but doesn't feel likely right now.

ok thank your for your views

Note: See TracTickets for help on using tickets.
Back to Top