Node annotations #3396
Node annotations #3396
Conversation
| if type(state_change) == Block: | ||
| assert isinstance(state_change, Block), MYPY_ANNOTATION |
There was a problem hiding this comment.
Hmm but why is the assert needed here and in all the files below? Shouldn't the if type(state_change) == X: be enough?
There was a problem hiding this comment.
It should, but it isn't. mypy does not detect it.
There was a problem hiding this comment.
That's funny. I guess shortcomings of mypy they will hopefully eventually fix. Or ... could it be a subtle different of type() vs isinstance()? I am just theorizing here. Did not test this.
There was a problem hiding this comment.
Hmm actually my theory seems to be correct. Let me open a PR for this.
This only throws warning for the type() check:
from typing import Union
class Boo(object):
a = 1
class Foo(object):
b = 2
def fun(f: Union[Foo, Boo]):
if type(f) == Foo:
print(f.b)
if isinstance(f, Boo):
print(f.a)| @@ -528,7 +528,7 @@ def from_dict(cls, data: Dict[str, Any]) -> 'TokenNetworkState': | |||
| serialization.identity, | |||
| data['partneraddresses_to_channelidentifiers'], | |||
| ) | |||
| restored.partneraddresses_to_channelidentifiers = defaultdict( | |||
| restored.partneraddresses_to_channelidentifiers = defaultdict( # type: ignore | |||
There was a problem hiding this comment.
What was this complaining about?
There was a problem hiding this comment.
type mismatch. defaultdict is not a generic.
edit: I tried to fix the annotation, but at some point I just gave up and decided to leave it for the future, eventually defaultdict should be made generic too.
| ST = TypeVar('ST', bound=State) | ||
|
|
||
|
|
||
| class TransitionResult(Generic[ST]): # pylint: disable=unsubscriptable-object |
There was a problem hiding this comment.
Nice use of generic. Note that at the next release of pylint we can remove this disable pragma. This is a bug on the pylint side: pylint-dev/pylint#2416
|
@hackaugusto Have a look at this when you can. Build is now green. |
Codecov Report
@@ Coverage Diff @@
## master #3396 +/- ##
==========================================
- Coverage 75.92% 75.87% -0.06%
==========================================
Files 97 97
Lines 12565 12604 +39
Branches 1760 1760
==========================================
+ Hits 9540 9563 +23
- Misses 2396 2410 +14
- Partials 629 631 +2
Continue to review full report at Codecov.
|
The utility has to be made generic for the check can work
LefterisJP
force-pushed
the
node_annotations
branch
from
5ffd579 to
9830843
Compare
|
@LefterisJP thanks, looks good to me. However, I cannot approve my own PR, so if it's also good for you, please accept it :) |
There was a problem hiding this comment.
I am assuming that @LefterisJP is good with this PR... from my side it's LGTM.
|
@rakanalh @LefterisJP yeah yeah sorry guys, forgot to approve since I was the last commiter in the branch. Apologies. |
This was prompted from the discussion made [here](raiden-network#3396 (comment)) And the documentation of mypy also states that `isinstance()` checks should be preferred for the same reason. [Source](https://mypy.readthedocs.io/en/latest/common_issues.html#complex-type-tests)
No description provided.