Added kwargs to register_view()

[ydaniv]

This allows passing kwargs to the register_view() and then this dictionary is accessible as a 3rd item in the custom_list tuples.

Currently no tests added, we use it internally so would like to know if there's a general interest.

[jsocol]

Sorry, I'm not following. What exactly does this change or make possible?

[ydaniv]

This change allows passing extra keyword arguments to the register_view() and have these accessible in the views.

For example, we're using those to categorize different URLs in the custom_list.

Weird the tests fail, it works fine, I'll try fixing these. For some reason I get:

ImportError: No module named ree

when trying to run tests.

[ydaniv]

OK, I see tests fail for different reasons. They pass here for Django 1.9 and python 2.7.

[jsocol]

Can you tell me more about what you're trying to accomplish with this, though? Honestly, I can follow your comment and the code (it's making these kwargs available to the index template, not to each view) but I still don't get why, or what this lets you do.

It's failing on the flake8 check. That should be easy enough to fix. You can run flake8 locally with ./run.sh check. Also, I don't like capturing all remaining kwargs, because it makes it hard to safely add other new args/kwargs in the future.

[ydaniv]

Yes, forgot to push the flake8 fix.

And right, only the index view.

The idea is to allow extra context with each view in the list. Going back to the above example: Lets say I have view1 and view2, and I have a Grappelli dashboard on the index view. I want view1 to go to column 1 and view2 to be in column 2. So @register_view(..., column=1) decorates view1 and @register_view(..., column=2) decorates view2.

Then when I override index, going over the custom_list's tuples I can check the 3rd item's 'column' and place the link in the appropriate column.

I think this is the least obtrusive way to enhance these views.

[jsocol]

Sorry, I thought I'd put a comment here, but I guess I didn't post it.

To be honest, I don't really this as a super common or compelling use case, I don't know if it's worth supporting so directly.

But I can see a few ways of getting similar functionality with smaller, more generic changes to AdminPlus. I think the most straight forward would be: annotate the view objects themselves and make them available to the template. For example:

def my_custom_view(request):
    # do fancy things
    return render(request, 'myadmintemplate.html')
my_custom_view.column = 1
admin.site.register_view('custom-path', my_custom_view)

In AdminPlusMixin.index(), we'd just change the tuples to custom_list.append((path, name, view)). This supports getting the column data, but it also supports, for example, getting the __doc__ attribute to use as "help" content, or whatever else.

And it does it without making all future changes to register_view breaking. (Once **kwargs gets passed to something, adding any other real kwarg breaks that behavior.)

[ydaniv]

Fair enough, I guess I can live with just setting an attribute on the view function.

But it would be nice, though, to also pass the url name, or at least the view, along in the custom_list for reverse()ing the url, since the path is not exactly the url.

[jsocol]

But it would be nice, though, to also pass the url name, or at least the view, along in the custom_list for reverse()ing the url, since the path is not exactly the url.

Could do that, but the urlname is not a required param (just as name= isn't required for django.conf.urls.url()), so it's not reliable. The index view can't depend on it, and so it doesn't seem worth including to me, the more I think about it.

[ydaniv]

Well then, what's the point in custom_list in the context if it's not even usable?

[jsocol]

It's used in the template to render the list?

Sent from my phone, please excuse the occasional typo.

On Mon, Dec 14, 2015 at 12:32 PM, Yehonatan Daniv notifications@github.com wrote:

Well then, what's the point in custom_list in the context if it's not even usable?

Reply to this email directly or view it on GitHub: https://github.com/jsocol/django-adminplus/pull/36#issuecomment-164503580

[ydaniv]

Yes, you get the titles (names), but sometimes you need the ability to customize/override stuff.

Anyhow, I'll close this PR since we've already concluded the main purpose here isn't going to be merged.

[jsocol]

Well then, what's the point in custom_list in the context if it's not even usable?

I'm going to guess I misinterpreted this question?

We pass custom_list to the index template to render the list of views. It doesn't get used anywhere else. custom_list is "usable" because it gets us that list. But because it's not used anywhere else, there's a good reason not to put extra stuff into it that isn't used to render the list.

I'm willing to consider making the view object itself available, since, like I said, it's a nice generic solution that enables a number of things with a single breaking API change, for people who've extended adminplus/index.html.

The urlname option does what url(name=) has always done: make it possible to use reverse / {% url %} to get the URL from somewhere else. So if you had two admin views and wanted one to link to the other, you could use urlname= to do it. But urlname is not required, so it cannot used by the index template, so there's little reason to include it in custom_list.

[ydaniv]

I see. Adding the view will also help a lot. The thing is, when using grappelli the dashboard module overrides the init (index) view and takes the context, so if it's possible to get the views and all related context there, and keep the code DRY, i.e. not repeat url names in multiple places in the code, would be great.

Currently I have to create another dictionary to map views by name to url names and view instances, which kinda sucks.

And on a side note, I don't see how adding extra non-mandatory kwargs will break current API, but maybe I just didn't get it.

[jsocol]

when using grappelli

I still have no idea how AdminPlus and grappelli work together at all and honestly supporting that integration isn't high on my list of priorities.

Currently I have to create another dictionary to map views by name to url names and view instances, which kinda sucks.

The urlname values aren't reliable, I'd encourage you to just rely on the path, unless there's some reason that isn't actually working?

And on a side note, I don't see how adding extra non-mandatory kwargs will break current API, but maybe I just didn't get it.

It doesn't break the API now, but if we ever add another real kwarg, that would be a breaking change. Say a function has this API:

def foo(a=1, b=2, **kwargs):

then it changes to:

def foo(a=1, b=2, c=3, **kwargs):

Anything that was passing c='bar' and relying on it ending up in kwargs has just been broken. This isn't a huge issue, though, because it's easy to add some other parameter that can be a dict, e.g.

def foo(a=1, b=2, extra_context=None):

instead of using **kwargs. Of course, in this specific case, when you go to name this kwarg, the start of the problem becomes more visible: extra_context is misleading because it implies it somehow gets injected into the view context, so it's really something like index_list_extra_context, which is incredibly specific and solves single, rare use case.

[ydaniv]

Works perfectly well with grappelli.

Actually using path isn't DRY and isn't very much in the spirit of Django. The Django way is reverse()ing the name of the url.

[jsocol]

Works perfectly well with grappelli.

Great! But supporting it (fixing bugs, making changes to make it work better) still doesn't rate very highly for me, unfortunately.

Actually using path isn't DRY and isn't very much in the spirit of Django. The Django way is reverse()ing the name of the url.

1. name is not a required parameter to url(),
2. so urlname is not a required parameter to register_view() here,
3. so it's not something you can rely on,
4. and so the index template needs to be able to fall back to using the path anyway.

It's really not a DRYness issue. Using reverse in the index template here sometimes just makes the template more complex, with more code. If there's not something broken, I'm disinclined to write and maintain extra code for no benefit.