Opened 4 years ago

Closed 4 months ago

Last modified 4 months ago

#16213 closed New feature (fixed)

View collections in the generic views

Reported by: hvdklauw Owned by: auvipy
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 auvipy)

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 hvdklauw 4 years ago.
Added some changes, added a mixin for the crud views to add extra functionality.
16213.2.diff (6.9 KB) - added by hvdklauw 4 years ago.
Some cleanup

Download all attachments as: .zip

Change History (24)

comment:1 Changed 4 years ago by hvdklauw

  • 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 4 years ago by hvdklauw

  • Keywords dceu2011 added

comment:3 Changed 4 years ago by hvdklauw

  • Cc hvdklauw added

comment:4 Changed 4 years ago by hvdklauw

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

comment:5 Changed 4 years ago by hvdklauw

  • Cc hvdklauw removed

comment:6 Changed 4 years ago by hvdklauw

  • Cc hvdklauw@… added

comment:7 Changed 4 years ago by jezdez

  • Triage Stage changed from Unreviewed to Accepted

I love it!

comment:8 Changed 4 years ago by lukeplant

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

Right.. I'll take that into account

comment:10 Changed 4 years ago by hvdklauw

Bah, wasn't logged in.

Changed 4 years ago by hvdklauw

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

Changed 4 years ago by hvdklauw

Some cleanup

comment:11 Changed 4 years ago by bpeschier

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 4 years ago by hvdklauw

  • milestone set to 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 4 years ago by jacob

  • milestone 1.4 deleted

Milestone 1.4 deleted

comment:14 Changed 3 years ago by abki

  • Cc amirouche.boubekki@… added

comment:15 Changed 3 years ago by abki

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 2 years ago by tomchristie

  • Cc tom@… added

comment:17 Changed 2 years ago by tomchristie

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 4 months ago by auvipy

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

comment:19 Changed 4 months ago by auvipy

  • Owner changed from hvdklauw to auvipy
  • Status changed from new to assigned
  • Version changed from 1.3 to master

comment:20 Changed 4 months ago by auvipy

  • Description modified (diff)

comment:21 follow-up: Changed 4 months ago by tomchristie

  • Resolution set to fixed
  • Status changed from assigned to closed
  • Triage Stage changed from Accepted to Someday/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 4 months ago by auvipy

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