Opened 16 years ago

Closed 12 years ago

Last modified 12 years ago

#5968 closed New feature (fixed)

Registering/Unregistering multiple models fails

Reported by: Anders Olsson Owned by: Aymeric Augustin
Component: contrib.databrowse Version: dev
Severity: Normal Keywords:
Cc: Triage Stage: Ready for checkin
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

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.

Attachments (6)

allowiterable.diff (1.3 KB ) - added by Anders Olsson 16 years ago.
allowiterable_2.diff (1006 bytes ) - added by Anders Olsson 16 years ago.
5968-1.diff (1.2 KB ) - added by Matt McClanahan 16 years ago.
Patch using the same test as AdminSite.register
5968-2.diff (2.2 KB ) - added by Matt McClanahan 16 years ago.
Add doc update
5968-3.diff (5.6 KB ) - added by jamesp 13 years ago.
Adding tests
5968-4.patch (6.0 KB ) - added by Aymeric Augustin 12 years ago.

Download all attachments as: .zip

Change History (25)

by Anders Olsson, 16 years ago

Attachment: allowiterable.diff added

comment:1 by Malcolm Tredinnick, 16 years ago

Patch needs improvement: set

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)

comment:2 by Anders Olsson, 16 years ago

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?

by Anders Olsson, 16 years ago

Attachment: allowiterable_2.diff added

comment:3 by Malcolm Tredinnick, 16 years ago

Needs documentation: set
Triage Stage: UnreviewedAccepted

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.

comment:4 by Anders Olsson, 16 years ago

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.

comment:5 by Matt McClanahan, 16 years ago

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

isinstance(model_or_iterable, ModelBase)

by Matt McClanahan, 16 years ago

Attachment: 5968-1.diff added

Patch using the same test as AdminSite.register

by Matt McClanahan, 16 years ago

Attachment: 5968-2.diff added

Add doc update

comment:6 by Matt McClanahan, 16 years ago

milestone: 1.0
Needs documentation: unset
Patch needs improvement: unset

comment:7 by Malcolm Tredinnick, 16 years ago

milestone: 1.0post-1.0

This is a feature addition, not a bug fix.

comment:8 by (none), 15 years ago

milestone: post-1.0

Milestone post-1.0 deleted

comment:9 by Gabriel Hurley, 13 years ago

Severity: Normal
Type: New feature

comment:10 by Julien Phalip, 13 years ago

Easy pickings: unset
Needs tests: set

by jamesp, 13 years ago

Attachment: 5968-3.diff added

Adding tests

comment:11 by jamesp, 13 years ago

UI/UX: unset

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

comment:12 by Preston Holmes, 12 years ago

Databrowse is now deprecated, see #16907

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

comment:13 by Paul McMillan, 12 years ago

Needs tests: unset
Patch needs improvement: set

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.

comment:14 by Aymeric Augustin, 12 years ago

Patch needs improvement: unset

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.

by Aymeric Augustin, 12 years ago

Attachment: 5968-4.patch added

comment:15 by Aymeric Augustin, 12 years ago

Owner: changed from nobody to Aymeric Augustin

comment:16 by Claude Paroz, 12 years ago

Triage Stage: AcceptedReady for checkin

It seems in good shape! Thanks.

comment:17 by Aymeric Augustin, 12 years ago

Resolution: fixed
Status: newclosed

In [17405]:

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

comment:18 by Aymeric Augustin, 12 years ago

In [17406]:

Added basic tests for databrowse. Refs #5968.

comment:19 by Aymeric Augustin, 12 years ago

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

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