Registering/Unregistering multiple models fails

[Anders Olsson]

Registering and unregistering multiple models fails. Calling for example

databrowse.site.register([model1, model2])

triggers this error: issubclass() arg 1 must be a class.

The patch fixes this by checking if the argument is a type before calling issubclass. The call to issubclass is now not strictly necessary (unless someone would want to pass in an iterable of models that is also type) but I kept it for clarity.

[Malcolm Tredinnick]

Checking for something being an instance of type is usually a sign of things going wrong. What are you actually trying to achieve here? Allowing an iterable as an argument? If so, then check to see if it's an iterable, not if it's a type, which is way too broad.

However, even that is probably not the best approach. If you're going to try and allow multiple models to be registered, just let register() take a *args parameter so the user doesn't have to needlessly wrap things in a tuple or list and can just write

databrowse.site.register(model1, model2)

[Anders Olsson]

The intent of the original author was to allow either a Model or an iterable of Models, but the check for identifying the actual case was faulty. An *args parameter would probably be nicest but the function has this signature, which doesn't make that a pretty option.

def register(self, model_or_iterable, databrowse_class=None, **options):

I have changed the patch to check if it's not an iterable instead of checking if it's a Model. By the way, is hasattr(o, "iter") the best way to check for an iterable?

[Malcolm Tredinnick]

Since you're doing a for-loop over the iterator, it must have an __iter__ or __getitem__ method, so the test you're doing is fine.

Leave the patch as it is for now, but I'm still tempted to move to the *args format. It would be mostly backwards compatible, the only difference being that databrowse_class would have to be specified as a keyword argument, but we can enforce that and that parameter isn't documented, so arguably isn't part of the public API (yet, at least).

I'm still a little concerned about how you're going about the test here. Currently we check that the input must be a model. This patch changes that so that site.register(int) won't raise the error it does now and will only cause problems later. I think it's important to keep the test for models in there somewhere. Presumably the end result is to allow multiple models to be registered at once. The iterable is just a way to achieve that, not a requirement, right? After all, given an iterable, you can still make it work with the *args format by calling register(*list(my_iter)). Thus my slight preference for the *args version when we can just iterate over args inside the register() method.

Anyway, let's leave it as is for now and we can tweak it when it comes time to commit. I think the idea is reasonable, however we end up imlpementing it.

[Anders Olsson]

Yes I believe it's just a convenience for adding several models at once. The use case where you already have a list of Models is probably rare (and if you did you could just use the * syntax in the call). Verifying that the arguments really are Models might be nice since this is the main public api of databrowse.

If we were to rethink the api, maybe it would make sense to provide registering at the application level in addition to individual models. I imagine this is how it looks for many users:

from myapp.models import ModelA, ModelB, ModelC, ModelD, ModelE
databrowse.site.register(ModelA)
databrowse.site.register(ModelB)
databrowse.site.register(ModelC)
databrowse.site.register(ModelD)
databrowse.site.register(ModelE)

or with the undocumented iterable approach:

from myapp.models import ModelA, ModelB, ModelC, ModelD, ModelE
databrowse.site.register([ModelA, ModelB, ModelC, ModelD, ModelE])

maybe something like this would be a nice alternative:

import myapp
databrowse.site.register_application(myapp)

Makes sense considering a main use case of databrowse is to provide a quick overview of the data for developers.

[Matt McClanahan]

Is there any reason not to fix this bug by using the same test that AdminSite uses?

isinstance(model_or_iterable, ModelBase)

[Matt McClanahan]

Patch using the same test as AdminSite.register

[Matt McClanahan]

Add doc update

[Malcolm Tredinnick]

This is a feature addition, not a bug fix.

[(none)]

Milestone post-1.0 deleted

[jamesp]

Adding tests

[jamesp]

Updated to trunk@[16797] and added tests (I hope checking to ensure models were correctly added to databrowse.site.registry is adequate).

[Preston Holmes]

Databrowse is now deprecated, see #16907

however this has a recent, solid patch, and should still be considered for inclusion while databrowse remains

[Paul McMillan]

I don't see anything wrong with this patch, and will be happy to add it as a minor improvement even though databrowse is deprecated. The tests are ok (since databrowse is otherwise untested). Unfortunately, the patch needs to be updated again to catch the pending deprecation warning. Feel free to mark it as RFC again once that's taken care of.

[Aymeric Augustin]

While the current code apparently intends to make it possible to register or unregister multiple models at once by passing a list, this ticket shows that it doesn't work, and it was never documented.

Like Malcolm, I'd prefer to move to the *args format, as demonstrated in the patch I'm attaching. Also, contrib apps bundle their own tests.

[Claude Paroz]

It seems in good shape! Thanks.

[Aymeric Augustin]

In [17405]:

Fixed #5968 -- Allowed (un-)registering with databrowse several models at once.

[Aymeric Augustin]

In [17406]:

Added basic tests for databrowse. Refs #5968.

[Aymeric Augustin]

The changes to tests/runtests.py should have gone in the second commit, sorry.