misc. move typing improvements, WIP #195
Conversation
Codecov ReportAttention:
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. Additional details and impacted files@@ Coverage Diff @@
## master #195 +/- ##
==========================================
+ Coverage 81.00% 81.35% +0.34%
==========================================
Files 47 47
Lines 2517 2548 +31
Branches 365 362 -3
==========================================
+ Hits 2039 2073 +34
+ Misses 383 382 -1
+ Partials 95 93 -2
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
duecredit/collector.py
Outdated
| from functools import wraps | ||
| from typing import Any, Dict, List, Optional, Tuple, Union | ||
| from typing import Any, Dict, List, Optional, Tuple, Union, TYPE_CHECKING |
There was a problem hiding this comment.
You're still using Dict, List, Tuple, etc. instead of dict, list, tuple, etc., which you can use now thanks to from __future__ import annotations. Please switch. (Pyupgrade should convert automatically now that the files contain from __future__ import annotations.)
There was a problem hiding this comment.
I first modified only 1 file this way to let CI run.
duecredit/tests/test__main__.py
Outdated
| @@ -40,8 +39,8 @@ def test_main_version(monkeypatch: 'MonkeyPatch') -> None: | |||
|
|
|||
|
|
|||
| def test_main_run_a_script( | |||
| tmpdir: 'TempdirFactory', | |||
| monkeypatch: 'MonkeyPatch' | |||
| tmpdir: TempPathFactory, | |||
There was a problem hiding this comment.
tmpdir is a py.path.local.
duecredit/cmdline/cmd_summary.py
Outdated
| @@ -20,10 +22,13 @@ | |||
|
|
|||
| __docformat__ = 'restructuredtext' | |||
|
|
|||
| if TYPE_CHECKING: | |||
| import argparse | |||
There was a problem hiding this comment.
There's no downside to importing argparse. Why gate it behind if TYPE_CHECKING?
duecredit/cmdline/cmd_test.py
Outdated
|
|
||
|
|
||
| if TYPE_CHECKING: | ||
| import argparse |
There was a problem hiding this comment.
Again, no reason for if TYPE_CHECKING.
duecredit/entries.py
Outdated
| self._rawentry = rawentry | ||
| self._key = key or rawentry.lower() | ||
|
|
||
| def __eq__(self, other: object) -> bool: | ||
| if not isinstance(other, DueCreditEntry): | ||
| raise NotImplemented | ||
| raise NotImplementedError |
There was a problem hiding this comment.
| raise NotImplementedError | |
| return NotImplemented |
| self._rawentry = rawentry | ||
| self._key = key or rawentry.lower() | ||
|
|
||
| def __eq__(self, other: object) -> bool: | ||
| if not isinstance(other, DueCreditEntry): | ||
| raise NotImplemented | ||
| return NotImplemented |
There was a problem hiding this comment.
I don't think it makes sense to return here
There was a problem hiding this comment.
It does make sense: https://docs.python.org/3/library/constants.html#NotImplemented
There was a problem hiding this comment.
ah -- cool, learned something new again, should have checked.
FWIW - here is a trial on a simple case
❯ python -m trace --trace notimpl.py
--- modulename: notimpl, funcname: <module>
notimpl.py(1): class A:
--- modulename: notimpl, funcname: A
notimpl.py(1): class A:
notimpl.py(2): def __eq__(self, other):
notimpl.py(8): print(A() == A())
--- modulename: notimpl, funcname: __eq__
notimpl.py(3): if isinstance(other, A):
notimpl.py(4): return True
True
notimpl.py(9): print(A() == 1)
--- modulename: notimpl, funcname: __eq__
notimpl.py(3): if isinstance(other, A):
notimpl.py(6): return NotImplemented
False
notimpl.py(10): print(A() != 1)
--- modulename: notimpl, funcname: __eq__
notimpl.py(3): if isinstance(other, A):
notimpl.py(6): return NotImplemented
True
❯ cat notimpl.py
class A:
def __eq__(self, other):
if isinstance(other, A):
return True
else:
return NotImplemented
print(A() == A())
print(A() == 1)
print(A() != 1)I will revert that f470c3e I merged in rush, directly in master
Hi,
I'm back working on this. I don't mind doing the +
from __future__ import-Union & Optionalby hand if pyupgrade can't.#191