Code

Opened 3 years ago

Closed 3 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 3 years ago.

Download all attachments as: .zip

Change History (7)

Changed 3 years ago by dimier

comment:1 Changed 3 years ago by lukeplant

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset
  • Resolution set to invalid
  • Status changed from new to closed

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 3 years ago by lukeplant

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 3 years ago by Alex

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

comment:4 Changed 3 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 3 years ago by dimier

  • Resolution invalid deleted
  • Status changed from closed to reopened

comment:6 Changed 3 years ago by russellm

  • Resolution set to wontfix
  • Status changed from reopened to closed

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

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.