Closed Bouni closed 6 months ago
Coverage Report
File Stmts Miss Cover Missing luxtronik __init__.py 143 110 23% 39–55, 62–67, 70, 74–79, 84–88, 101–104, 111, 115, 119, 123, 134, 137–140, 143–163, 166–181, 184–201, 204–219, 223–225, 229–230, 234–235 __main__.py 21 21 0% 3–49 datatypes.py 275 1 99% 82 discover.py 40 34 15% 21–69 luxtronik/scripts dump_changes.py 44 44 0% 5–93 dump_luxtronik.py 28 28 0% 5–64 TOTAL 638 238 63%
Tests | Skipped | Failures | Errors | Time |
---|---|---|---|---|
120 | 0 :zzz: | 0 :x: | 0 :fire: | 0.688s :stopwatch: |
I tried really hard to get rid of this 1% but had no luck so far.
There is something I don't understand about child classes and instanceof which always return the base class ...
@kbabioch Can you please take a look at this?
Can't you just compare a Base("bar")
with something completely else, e.g., the integer 1
?
I just don't understand what was the initial motivation for the check if the class is not equal to Base here.
In my opinion we could remove this check altogether.
A child class still has all three attributes that are compared below that if I understand it right.
@kbabioch can you give us some clarification of what your intention was with that check?
I think that the rational for the test isinstance(other, Base)
is that if other
is not an instance of Base
, we cannot access other.value
, other.datatype_class
and other.datatype_unit
in the lines below and this would raise an error.
The same comment would apply to __lt__
.
However, python3
itself raises a TypeError
on "barfoo" < 1
, but returns False
on "barfoo" == 1
. I think that we should follow the same principle?
Ok, so I probably misundertood this. I thougt the check should return False
if other
is a subclass of Base, like Frequency
for example.
So the TypeError
comes from python but we need to take care of the return False for implemented methods like __eq__
, __lt__
and so on?
So the
TypeError
comes from python
Which TypeError
do you refer to?
but we need to take care of the return False for implemented methods like
__eq__
,__lt__
and so on?
I think that I am not sure what you mean.
From my side:
__eq__
method is fine, we just need a test for the return False
, e.g., Base("barfoo") == 1
.__lt__
should be improved and should raise a TypeError
if the two compared objects are not comparable, e.g., if not isinstance(other, Base)
or if self.datatype_class != other.datatype_class
or if self.datatype_unit != other.datatype_unit
.I think I didn't understand what you iniztially wrote the right way but no I think I got it 😅
So for __eq__
we return false but for other methods like __lt__
, __gt__
we raise an Exception instead.
But I think that should not be the scope of this PR but a seperate issue instead. I'll open one for that.
I think its better to create a new PR for more unit tests to make them easy to review.
@gerw Yes if you consider squashing helpful here, go for it (never used it before 😅 )
I started adding more unit tests to get more coverage.