Opened 6 years ago

Closed 6 years ago

#14814 closed (wontfix)

Check for file binding in FieldFile.delete()

Reported by: dimier Owned by: nobody
Component: Database layer (models, ORM) Version: master
Severity: Keywords: field file, FieldFile
Cc: Triage Stage: Unreviewed
Has patch: no Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: UI/UX:

Description

Atteched patch adds a simple check for file association in FieldFile.delete method.

Attachments (1)

r14751_field_file.diff (487 bytes) - added by dimier 6 years ago.

Download all attachments as: .zip

Change History (7)

Changed 6 years ago by dimier

Attachment: r14751_field_file.diff added

comment:1 Changed 6 years ago by Luke Plant

Needs documentation: unset
Needs tests: unset
Patch needs improvement: unset
Resolution: invalid
Status: newclosed

I don't understand the point of this patch. It would simply make calling FieldFile.delete() a no-op, rather than an error. But it should be an error, since calling an instance method without passing an instance is a mistake.

If there is something I've missed, please explain and re-open.

comment:2 Changed 6 years ago by Luke Plant

Sorry, that should be: it would make FieldFile.delete(None) a no-op. It isn't obvious why you would want to do that (instead of just, um, not calling that method).

comment:3 Changed 6 years ago by Alex Gaynor

self evaluates to False if there's no file attached to the wrapper.

comment:4 Changed 6 years ago by dimier

Thanks, lukeplant and Alex.

Of course, I didn't call FieldFile.delete() directly.

First, calling delete() without associated file will raise OSError exception:

OSError: [Errno 21] Is a directory: '/home/dimier/testproject/media'

And no exception will be raised if I do album.cover_img = None before delete(). It's ambiguously, I think.

Second, the point of this patch is to minimize number of checks to delete a file.

For example, if I want to update an album cover I need to write the following code:

if album.cover_img:
    album.cover_img.delete()
album.cover_img = new_cover_path

In some models in my project I have several image fields needed to update and I get/set them through getattr/setattr, so without described check the code become more readable.
If I just set album.cover_img to a new value without delete(), previous associated file will not be deleted.

So, the following will be simpler:

album.cover_img.delete()
album.cover_img = new_cover_path

This small patch is just my first experience in contribution to open-source and I have another improvements to publish. If the patch is useless, just leave the ticket closed.

comment:5 Changed 6 years ago by dimier

Resolution: invalid
Status: closedreopened

comment:6 Changed 6 years ago by Russell Keith-Magee

Resolution: wontfix
Status: reopenedclosed

I am in agreement with Luke and Alex. It isn't a good thing to be silently hiding an error check.

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