Basefix #67

squirrelo · 2014-06-02T15:55:10Z

Now check types are the same for base class as objects that were of different type but same ID were considered equal.

Also adds skeleton for data object so tests don't fail.

wasade · 2014-06-02T15:56:14Z

qiita_db/base.py

@@ -80,7 +80,7 @@ def __init__(self, id_):
        self._id = id_

    def __eq__(self, other):
-        return other._id == self._id
+        return other._id == self._id and type(self) == type(other)


doesn't _id equality imply they're the same?

Nope, accidentally compared a RawData object and PreprocessedData object with the same id and they were considered equal. Need the type check. It's because this is in the base class I think.

since _id isn't globally unique then, how confident are you that create and delete work as expected?

What do you mean by that? Create and delete are defined by each child object, so they should work regardless of this issue. This is more for comparison after creation.

it looks like, right now, you can delete something of type foo using an instance of type bar. is that intended?

Scratch that, you pass an ID of the thing you want to delete as a class method. You can't delete foo with bar thing because of this. All you are using is the integer id.

Should we change delete to be instance method instead of a class method? This will solve this issue.

This way, de delete functions won't need to get an id, they will get them from the instance.

that seems reasonable, is there a need to pass an _id to delete?

On Mon, Jun 2, 2014 at 12:06 PM, josenavas notifications@github.com wrote:

In qiita_db/base.py:

@@ -80,7 +80,7 @@ def init(self, id_):
self.id = id

def __eq__(self, other):

return other._id == self._id

return other._id == self._id and type(self) == type(other)

This way, de delete functions won't need to get an id, they will get them
from the instance.

—
Reply to this email directly or view it on GitHub
https://github.com/biocore/qiita/pull/67/files#r13298288.

When it was a class method it needed an id. I won't be needed once it is an
instance method.

2014-06-02 12:13 GMT-06:00 Daniel McDonald notifications@github.com:

In qiita_db/base.py:

@@ -80,7 +80,7 @@ def init(self, id_):
self.id = id

def __eq__(self, other):

return other._id == self._id

return other._id == self._id and type(self) == type(other)

that seems reasonable, is there a need to pass an _id to delete?
… <#1465dc9952698c34_>
On Mon, Jun 2, 2014 at 12:06 PM, josenavas notifications@github.com
wrote: In qiita_db/base.py: > @@ -80,7 +80,7 @@ def init(self, id_): >
self.id = id > > def eq(self, other): > - return other._id ==
self._id > + return other._id == self._id and type(self) == type(other)
This way, de delete functions won't need to get an id, they will get them
from the instance. — Reply to this email directly or view it on GitHub <
https://github.com/biocore/qiita/pull/67/files#r13298288>.

—
Reply to this email directly or view it on GitHub
https://github.com/biocore/qiita/pull/67/files#r13298667.

Jose Navas

josenavas · 2014-06-02T16:51:32Z

👍 Good catch on the __eq__ stuff. Since all our classes are based on this class, the check for type is needed. Thanks @squirrelo

@wasade if you don't see any further issues, feel free to merge it

ElDeveloper · 2014-06-02T17:34:14Z

qiita_db/base.py

@@ -80,7 +80,7 @@ def __init__(self, id_):
        self._id = id_

    def __eq__(self, other):
-        return other._id == self._id


My suggestion here is that we separate checks one per if statement. This is very helpful for debugging.

I don't see this point... it will return true or false anyways, you're not going to know in which if statement it was providing the output.

Yes, but if you add a print statement inside the if conditional then you
can quickly check that as opposed to having to break the contitional at
the botton in mutliple lines.

On (Jun-02-14|10:37), josenavas wrote:

@@ -80,7 +80,7 @@ def init(self, id_):
self.id = id

def __eq__(self, other):

return other._id == self._id

I don't see this point... it will return true or false anyways, you're not going to know in which if statement it was providing the output.

Reply to this email directly or view it on GitHub:
https://github.com/biocore/qiita/pull/67/files#r13296674

adds clarity in the traceback as well

Oh, ok, thanks!

OK, broken up.

Thanks!

On (Jun-02-14|11:10), Joshua Shorenstein wrote:

@@ -80,7 +80,7 @@ def init(self, id_):
self.id = id

def __eq__(self, other):

return other._id == self._id

OK, broken up.

Reply to this email directly or view it on GitHub:
https://github.com/biocore/qiita/pull/67/files#r13298524

coveralls · 2014-06-02T18:16:40Z

Coverage decreased (-0.61%) when pulling e3db027 on squirrelo:basefix into e448c10 on biocore:master.

coveralls · 2014-06-02T18:20:34Z

Coverage increased (+0.32%) when pulling d8d8757 on squirrelo:basefix into e448c10 on biocore:master.

josenavas · 2014-06-02T18:24:50Z

qiita_db/base.py

@@ -80,7 +80,10 @@ def __init__(self, id_):
        self._id = id_

    def __eq__(self, other):
-        return other._id == self._id
+        if other._id == self._id:
+            if type(self) == type(other):


I'd say that it makes sense to broke things up on the other order, first by type and then by id. What if you pass an integer instead of a subclass? Currently it will raise an error, while if you change the order it will just return false.

Yeah, like this:

if other._id != self._id: return False if type(self) != type(other): return False return True

It will still fail, it should be this way:

if type(self) != type(other): return False if other._id != self._id: return False return True

Ah, yes.

On (Jun-02-14|11:35), josenavas wrote:

@@ -80,7 +80,10 @@ def init(self, id_):
self.id = id

def __eq__(self, other):

return other._id == self._id

if other._id == self._id:

if type(self) == type(other):

It will still fail, it should be this way:

if type(self) != type(other): return False if other._id != self._id: return False return True

Reply to this email directly or view it on GitHub:
https://github.com/biocore/qiita/pull/67/files#r13299908

Eh, ok. Fixed.

coveralls · 2014-06-02T18:31:56Z

Coverage increased (+0.32%) when pulling 582031a on squirrelo:basefix into e448c10 on biocore:master.

josenavas · 2014-06-02T18:39:27Z

I know you haven't change it, but can you remove the @classmethod decorator from the delete function, as well as the cls parameters. Thanks.

squirrelo · 2014-06-02T18:40:16Z

Nevermind. Done.

coveralls · 2014-06-02T18:41:38Z

Coverage increased (+0.44%) when pulling 8901c3c on squirrelo:basefix into e448c10 on biocore:master.

josenavas · 2014-06-02T18:47:28Z

qiita_db/base.py

@@ -80,7 +79,11 @@ def __init__(self, id_):
        self._id = id_

    def __eq__(self, other):
-        return other._id == self._id
+        if other._id != self._id:


The order here is still wrong, check my previous comment.

ID is faster to check though, so probably faster this way. If the ID is different, it doesn't matter if it's the same type or not; it's considered not equal.

Faster, but it will raise an error instead of returning false if "other" is not a subclass of BaseClass. Please check all the comments that we make in the code.

ah, right. Didn't catch that was why the error was raised during the convo.

coveralls · 2014-06-02T18:51:15Z

Coverage increased (+0.44%) when pulling a2c5104 on squirrelo:basefix into e448c10 on biocore:master.

josenavas · 2014-06-02T18:53:05Z

It looks good to me now.

coveralls · 2014-06-02T18:55:57Z

Coverage increased (+0.44%) when pulling ac9863c on squirrelo:basefix into e448c10 on biocore:master.

squirrelo · 2014-06-02T18:57:30Z

I think this is good now.

josenavas · 2014-06-02T18:58:59Z

@wasade @ElDeveloper if it looks good to you, feel free to merge it.

Basefix

squirrelo added 2 commits June 2, 2014 09:52

fix base eq check and add data skeleton

2fe6fd3

Remove extra check

dde4f49

wasade reviewed Jun 2, 2014
View reviewed changes

ElDeveloper reviewed Jun 2, 2014
View reviewed changes

break up if into two statements

e3db027

test base eq ne

d8d8757

josenavas reviewed Jun 2, 2014
View reviewed changes

Fix docstring on classes

582031a

changed ifs

8901c3c

delete no longer classmethod

a2c5104

josenavas reviewed Jun 2, 2014
View reviewed changes

reorder if statements

ac9863c

wasade added a commit that referenced this pull request Jun 2, 2014

Merge pull request #67 from squirrelo/basefix

223b7f0

Basefix

wasade merged commit 223b7f0 into qiita-spots:master Jun 2, 2014

squirrelo deleted the basefix branch June 4, 2014 04:36

Basefix #67

Basefix #67

Conversation

squirrelo commented Jun 2, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

josenavas commented Jun 2, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Jun 2, 2014

coveralls commented Jun 2, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Jun 2, 2014

josenavas commented Jun 2, 2014

squirrelo commented Jun 2, 2014

coveralls commented Jun 2, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Jun 2, 2014

josenavas commented Jun 2, 2014

coveralls commented Jun 2, 2014

squirrelo commented Jun 2, 2014

josenavas commented Jun 2, 2014