Changes between Initial Version and Version 1 of Ticket #25374, comment 5


Ignore:
Timestamp:
Sep 10, 2015, 9:57:36 AM (9 years ago)
Author:
Simon Charette

Legend:

Unmodified
Added
Removed
Modified

  • Ticket #25374, comment 5

    initial v1  
    1 The main argument I see about running checks against instance instead of subclasses of `ModelAdmin` is the fact they are the actual objects that registered to the site, Django doesn't actually use `ModelAdmin` classes directly. This would be consistent with how checks are ran against `db.Field` instance compared to `db.Models` classes.
     1The main argument I see about running checks against instances instead of subclasses of `ModelAdmin` is the fact they are the actual objects registered to the site, Django doesn't actually use `ModelAdmin` classes directly. This would be consistent with how checks are ran against `db.Field` instance compared to `db.Models` classes.
    22
    3 If you take a look at the admin check methods signatures you can clearly see most of them can't be performed if they are not provided the `db.Model` the `ModelAdmin` must be bound to. The model bounding is something that happens at `ModelAdmin` initialization compared to `InlineModelAdmin` which is done at class definition. I think it would make more sense to run these checks against "bound" objects instead which is `ModelAdmin` instances and `InlineModelAdmin` classes.
     3If you take a look at the admin check methods signatures you can clearly see most of them can't be performed if they are not provided the `db.Model` the `ModelAdmin` must be bound to. The model bounding is something that happens at `ModelAdmin` initialization compared to `InlineModelAdmin` which is done at class definition. I think it would make more sense to run these checks against '''bound''' objects instead.
    44
    5 I would also argue that string representation of `ModelAdmin` should be changed to the reflect its full class path and the model it's bound to instead of its bound model `app_label` and the class name which doesn't make much sense anyway. Combined with the proposed changes it should make it easier to identify the check offending `ModelAdmin` instance given the `db.Model` it's bound to is displayed.
     5I would also argue that the string representation of `ModelAdmin` should be changed to the reflect its full class path and the model it's bound to instead of its bound model `app_label` and its class name which doesn't make much sense anyway. Combined with the proposed changes it should make it easier to identify the check offending `ModelAdmin` instance given the bound `db.Model` is also part of its string representation.
    66
    77As Tim said It wouldn't hurt writing to the mailing list to get extra feedback but IMHO we should tentatively accept this ticket as a cleanup/optimization of the admin checks.
     8
     9Edit: It looks like the string representation of `ModelAdmin` is not currently used in checks message but it's bound model is referred when it makes sense to.
Back to Top