Opened 23 months ago

Last modified 23 months ago

#26300 new Cleanup/optimization

Convert contrib.flatpages views to class-based views

Reported by: Tom Carrick Owned by: Tom Carrick
Component: contrib.flatpages Version: master
Severity: Normal Keywords:
Cc: Triage Stage: Someday/Maybe
Has patch: no Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

I'm intending on applying for GSoC 2016 and the wiki mentions it would be nice if I've done some work in the area - so this is my attempt to do so.

Summary speaks for itself. We should look at using a CBV for the flatpages view. I notice there is an extra internal view and I'll look into seeing if there's a nice way of integrating it into the CBV rather than just calling it from within it.

Change History (5)

comment:1 Changed 23 months ago by Tim Graham

Could you motivate the reason why they should become class-based views? Is it a common need to customize them? If so, how? As the views are only a handful of lines each, will a class-based approach be any simpler than writing a custom view?

Class-based views are not always better/simpler than function-based ones.

comment:2 Changed 23 months ago by Tom Carrick

I don't disagree that CBVs are not always better, but I think they may be here. Personally, I think there are two things here.

Firstly, maybe you disagree, but I think that having two views and several lines of comments to explain why there are two is kind of messy. I think that's valuable cleanup in itself.

Secondly, yeah, it may not be much simpler than writing a custom view, but I think it's certainly cleaner.

Quick example:

You have a contact page with some copy above it. You want to be able to change the copy without changing the code. This is a very common scenario I've dealt with a number of times. There are several solutions here.

  1. Make your own Page model, and a view for it, then use DetailView and FormMixin here. Not bad, but what if flatpages are suitable for you otherwise, it makes more sense to leverage them right?
  1. Use a regular FormView and something like django-flatblocks to put the copy in. Fine, but if you don't need flatblocks for anything else, it's kind of annoying.
  1. Use flatpages, have a separate view for the contact form, use a custom flatpage template and put your form in the template manually. Kind of awful in my view.
  1. Use flatpages model, but make your own view for it. Okay, kinda works, but do you really want to copy all the code from the flatblocks views for this? Violates DRY for me.

Let me know if there's a good solution I'm missing.

Proposed solution:

Inherit FlatPageDetailView (or whatever) and mix in FormMixin. Then just provide either a custom template with the form, or have your default flatpage template check for a form and render it if it exists. Seems like a nice solution in my view. It might make a good example for the docs, too.

comment:3 Changed 23 months ago by Tim Graham

Summary: Dogfood CBV in contrib.flatpagesConvert contrib.flatpages views to class-based views
Triage Stage: UnreviewedSomeday/Maybe

Thanks, I guess I'll have to see the final code to say for sure but it's good to see you've thought about it.

comment:4 Changed 23 months ago by Tom Carrick

Thanks Tim. Where can I go from here, should I work on an implementation and submit a PR or do we need discussion on the mailing list or anything?

comment:5 Changed 23 months ago by Tim Graham

I think seeing the code changes will help evaluate whether or not this will be useful.

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