Code

Opened 3 years ago

Closed 3 years ago

#16377 closed Cleanup/optimization (fixed)

"Conditionally disabling actions" documentation needs updating

Reported by: pgullekson@… Owned by: nobody
Component: Documentation Version: 1.3
Severity: Normal Keywords:
Cc: Triage Stage: Ready for checkin
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: yes UI/UX: no

Description (last modified by aaugustin)

The docs in question are:
https://docs.djangoproject.com/en/1.3/ref/contrib/admin/actions/#conditionally-enabling-or-disabling-actions

The suggestion is to use del actions['delete_selected'], which worked fine in Django 1.2. However, because of changes made to django.contrib.admin.options.get_actions, in the case that IS_POPUP_VAR == True, this del statement will result in an exception. Need a try/except or something.

Attachments (2)

16377.diff (570 bytes) - added by aaugustin 3 years ago.
16377.2.diff (622 bytes) - added by aaugustin 3 years ago.

Download all attachments as: .zip

Change History (11)

comment:1 Changed 3 years ago by aaugustin

  • Description modified (diff)
  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset

Fixed formatting (please use a little bit of wiki syntax and the preview to make your reports readable).

Changed 3 years ago by aaugustin

comment:2 Changed 3 years ago by aaugustin

  • Easy pickings set
  • Has patch set
  • Triage Stage changed from Unreviewed to Accepted

comment:3 Changed 3 years ago by julien

  • Patch needs improvement set

I think it would be a bit cleaner and read better using the conditional test: if actions.has_key('delete_selected'). What do you think?

comment:4 Changed 3 years ago by jezdez

Please don't use has_key. It's idiomatic to do:

if 'delete_selected' in actions:
    # whatever
Last edited 3 years ago by jezdez (previous) (diff)

comment:5 Changed 3 years ago by julien

Agreed, that's even better. Thanks for correcting me :p

Changed 3 years ago by aaugustin

comment:6 Changed 3 years ago by aaugustin

  • Patch needs improvement unset

I'm sure we could write a whole book on the respective merits of

mydict.pop('mykey', None)    # that's unpythonic.

vs.

if 'mykey' in mydict:
    del mydict['mykey']      # we're writing the dict and key names twice!

vs.

try:
    del mydict['mykey']
except KeyError:
    pass                     # four lines for this?

Anyway, here's an updated patch...

Now, back to triaging the unreviewed tickets queue.

comment:7 Changed 3 years ago by julien

  • Triage Stage changed from Accepted to Ready for checkin

Thanks for the updated patch. IMO, writing the key twice is the lesser evil in this case ;)

comment:8 Changed 3 years ago by jezdez

FTR, I've mentioned that because Alex specifically removed the use of has_key in r14392 to speed up Django a bit. I think it was regarding using Django on PyPy.

comment:9 Changed 3 years ago by jezdez

  • Resolution set to fixed
  • Status changed from new to closed

In [16567]:

Fixed #16377 -- Fixed docs about how to disable the admin actions conditionally to match the current code. Thanks, Aymeric Augustin.

Add Comment

Modify Ticket

Change Properties
<Author field>
Action
as closed
as The resolution will be set. Next status will be 'closed'
The resolution will be deleted. Next status will be 'new'
Author


E-mail address and user name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.