qiita
qiita copied to clipboard
qiita-spots
Should `delete` be an instance method instead of a classmethod?
I think it makes more sense to have it as an instance method rather than a class method.
@antgonza concern is:
pd.delete()
pd.whatever = 55
However, this is not solved with the current solution:
PreprocessedData.delete(ppd.id)
ppd.whatever = 55
IMOO, I think it will be safer being an instance method because in such case we can invalidate the .id attribute by setting it to -1 and then do some decorator magic to check that value before any function is executed and raise a useful error.
What do others think?
From the code examples it is not immediately clear, what the problem is. Would you mind clarifying?
On (Apr-07-15|13:45), josenavas wrote:
I think it makes more sense to have it as an instance method rather than a class method.
@antgonza concern is:
pd.delete() pd.whatever = 55However, this is not solved with the current solution:
PreprocessedData.delete(ppd.id) ppd.whatever = 55IMOO, I think it will be safer being an instance method because in such case we can invalidate the
.idattribute by setting it to-1and then do some decorator magic to check that value before any function is executed and raise a useful error.What do others think?
Reply to this email directly or view it on GitHub: https://github.com/biocore/qiita/issues/1045
I think the most immediate problem is that we can have an object that is referencing an object in the DB that does not exist:
o = DB_Object(id)
# Do some stuff with that instance here
DB_Object.delete(o.id)
# Do some other stuff with the instance here e.g.:
var = o.attribute
And the error that will be raised is dependent of what we are doing with that instance.
If we move it from a classmethod to an instance method, we can always raise a consistent error if we put some decorators on the functions/properties:
o = DB_Object(id)
# Do some stuff with that instance here
o.delete()
# Do some other stuff with the instance here e.g.:
var = o.attribute
IncompetentQiitaDeveloperError: the object has been deleted!
The other issue that I see was pointed out by @wasade a long time ago. We can call delete with a random identifier:
pd = ProcessedData(1)
RawData.delete(pd.id)
Study.delete(np.random.randint(low=0, high=10))
but if it is an instance method, you are exactly aware of which object you're deleting:
pd = ProcessedData(1)
pd.delete()
rd = RawData(1)
rd.delete()
And finally, I think that philosophically, the delete function makes more sense to be in the instance rather than in the class. I think this was an oversight in the original design, and it is not too late to change.
Hmmm, got it thanks a lot for the explanation. Would it make sense then
to use the del method? That way you can no longer access that
object and we avoid having to decorated things, etc. A problem though is
that since doing del pd only decrements the reference count of pd by
1, we might encounter situations where things are not actually deleted
i.e. a case where another object still keeps a reference to the
instantiated object (which shouldn't be the case), but ... maybe.
On (Apr-07-15|14:03), josenavas wrote:
I think the most immediate problem is that we can have an object that is referencing an object in the DB that does not exist:
o = DB_Object(id) # Do some stuff with that instance here DB_Object.delete(o.id) # Do some other stuff with the instance here e.g.: var = o.attributeAnd the error that will be raised is dependent of what we are doing with that instance.
If we move it from a classmethod to an instance method, we can always raise a consistent error if we put some decorators on the functions/properties:
o = DB_Object(id) # Do some stuff with that instance here o.delete() # Do some other stuff with the instance here e.g.: var = o.attribute IncompetentQiitaDeveloperError: the object has been deleted!The other issue that I see was pointed out by @wasade a long time ago. We can call delete with a random identifier:
pd = ProcessedData(1) RawData.delete(pd.id) Study.delete(np.random.randint(low=0, high=10))but if it is an instance method, you are exactly aware of which object you're deleting:
pd = ProcessedData(1) pd.delete() rd = RawData(1) rd.delete()And finally, I think that philosophically, the delete function makes more sense to be in the instance rather than in the class. I think this was an oversight in the original design, and it is not too late to change.
Reply to this email directly or view it on GitHub: https://github.com/biocore/qiita/issues/1045#issuecomment-90731825
@ElDeveloper Thanks for bringing that up. I was unsure about the use of __del__ so I read a couple of posts. I think we should not use it. The reason is that __del__ might be called at the end of the program/closure, so we don't want to remove it from the database! Note that __del__ is used to free up the memory used by that object; and we will be using it to delete the persistent storage - i.e. every time that we free up the memory for an object, we will be deleting it from the DB. This is not what we want, so I think that having the delete function as a instance method is the way to go.
@biocore/qiita-dev I would appreciate if we can reach a consensus here. The reason is that I would like to change this for alpha 0.2, before the code keeps growing...
Looking over the logic, I think having it an instance method is probably safer for the random ID passing issue stated above. This would force you to instantiate the object before deleting, so you know what you're deleting.
Not, it has not been addressed. I would like to have it addressed at some point, however, since I think it will make different sections of the code easier.
Group resolution: We should do it, low priority. Add something so if an instance has been deleted and it gets accessed, it raises an error.