Check for file binding in FieldFile.delete()

[dimier]

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

[Luke Plant]

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.

[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).

[Alex Gaynor]

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

[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.

[Russell Keith-Magee]

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