added type references to pager.py, mypy produced no errors #936
Conversation
There was a problem hiding this comment.
Thanks for working on this!
It looks like some builds are failing because list can't be subscripted, I added a note about why.
When I run mypy, I get a few errors
$ python -m mypy
bpython/pager.py:57: error: Item "None" of "Optional[IO[bytes]]" has no attribute "write"
bpython/pager.py:58: error: Item "None" of "Optional[IO[bytes]]" has no attribute "close"
bpython/_internal.py:7: error: Incompatible types in assignment (expression has type "Callable[[str, bool], None]", variable has type "Callable[[str], None]")
Found 3 errors in 2 files (checked 64 source files)
If you figure out how our environments are different let me know and maybe we can add it to some developer docs.
The bpython/_internal.py file is getting more strictly checked now because it uses page defined in pager.py. Apparently the real pydoc.pager doesn't take that second argument. I'd be fine with a type: ignore for that.
pydoc.pager = page # type: ignore
bpython/pager.py
Outdated
| @@ -30,29 +32,29 @@ | |||
| import shlex | |||
|
|
|||
|
|
|||
| def get_pager_command(default="less -rf"): | |||
| def get_pager_command(default: str = "less -rf") -> list[str]: | |||
There was a problem hiding this comment.
Unfortunately using list and dict in type annotations instead of typing.List and typing.Dict was added in 3.9 and bpython still supports 3.8 and 3.7, so we can't use these yet; you'll need to import List from typing and use that.
| """A more than dumb pager function.""" | ||
| if hasattr(pydoc, "ttypager"): | ||
| pydoc.ttypager(data) | ||
| else: | ||
| sys.stdout.write(data) | ||
|
|
||
|
|
||
| def page(data, use_internal=False): | ||
| def page(data: str, use_internal: bool = False) -> None: | ||
| command = get_pager_command() | ||
| if not command or use_internal: | ||
| page_internal(data) | ||
| else: | ||
| curses.endwin() | ||
| try: | ||
| popen = subprocess.Popen(command, stdin=subprocess.PIPE) |
There was a problem hiding this comment.
For me this needs a
| popen = subprocess.Popen(command, stdin=subprocess.PIPE) | |
| assert popen.stdin is not None | |
| popen = subprocess.Popen(command, stdin=subprocess.PIPE) |
to silence mypy errors about stdin maybe being None.
|
If you aren't able to get mypy to produce these errors locally, you can see them in the CI runs on this page. |
|
Linter workflow did fail, however it seemed to only be because I modified _internal.py by adding the suggested # type: ignore to pydoc.pager = page. |
Codecov Report
@@ Coverage Diff @@
## main #936 +/- ##
==========================================
+ Coverage 68.11% 68.65% +0.53%
==========================================
Files 61 62 +1
Lines 9244 9434 +190
==========================================
+ Hits 6297 6477 +180
- Misses 2947 2957 +10
Continue to review full report at Codecov.
|
|
Indeed, the |
|
Thanks for adding these types and fixing these CI issues, this looks good to me. The pypy3 CI failure is failing on main right now too. |
|
Glad to complete my first contribution to a project that I use daily. I
hope to take the things I learned about the development process and
continue to contribute in my own small way to this project. Any feedback
on how I have conducted my interactions with you are welcome. Thank you so
much for your time.
…On Tue, Nov 9, 2021 at 11:17 AM Thomas Ballinger ***@***.***> wrote:
Thanks for adding these types and fixing these CI issues, this looks good
to me. The pypy3 CI failure is failing on main right now too.
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#936 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/AMQSM4JJNZPRYEKCE3TUYZ3ULFCSRANCNFSM5HEVBCTA>
.
Triage notifications on the go with GitHub Mobile for iOS
<https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675>
or Android
<https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>.
|
First time adding to a project on github, started off small. Would really appreciate feedback.