Conversation
|
btw: https://github.com/davidfritzsche/pytest-mypy-testing/ might be interesting for testing the stubs. |
|
Awesome! Will really help with pytest typing. I have some general comments below, will follow up with concrete comments. First, I assume that given Given that, I think that the stubs should only really expose the public API, and not include internal/private details. And also, only importable items (functions, classes, etc.) should be included. Or if we want to expose private items, we should prefix them with py does some weird things with imports but the |
|
@bluetech |
| @@ -0,0 +1,8 @@ | |||
| from py._error import ErrorMaker | |||
There was a problem hiding this comment.
ErrorMaker is not exposed so I'd add it with underscore.
| @@ -0,0 +1,8 @@ | |||
| from py._error import ErrorMaker | |||
|
|
|||
| from . import path | |||
There was a problem hiding this comment.
Seems better to me not to import here, but instead handle as a sub-package in the stubs. That is, add py/path/__init__.pyi. (We might want to separate the stubs to some separate source tree, if that's even possible, so it doesn't affect the real package).
There was a problem hiding this comment.
We might want to separate the stubs to some separate source tree, if that's even possible, so it doesn't affect the real package
py/path is empty without the stubs.
Do you mean to keep py/path/*.pyi there then, or do you mean to use something like py/_typed/…?
The idea of using the main __init__.pyi was to see what's really exposed/used - but I guess since it is used from/via py/path that is something mypy looks at also already.
There was a problem hiding this comment.
I saw later that you already added py/path directory, so you can just delete this line already, as it's redundant with the subpackage.
My comment about "separate source tree" was that adding actual py/path directory might have some affect on runtime module resolution? With namespace packages and such? I.e. currently import py.path.foo goes thru the py import hackery to the private py._path package, but if you we add actual py/path directory does it break anything?
If not then great :)
There was a problem hiding this comment.
Not sure, but I assume it is fine, since there is only .pyi, no .py.
| from py._error import ErrorMaker | ||
|
|
||
| from . import path | ||
| from ._vendored_packages import iniconfig |
There was a problem hiding this comment.
Same for iniconfig and io re. subpackages.
| @@ -0,0 +1,72 @@ | |||
| import ctypes | |||
There was a problem hiding this comment.
The only types from this file that are exposed in py.io are the following, so I'd just leave those and delete everything else!
'TerminalWriter' : '._io.terminalwriter:TerminalWriter',
'ansi_print' : '._io.terminalwriter:ansi_print',
'get_terminal_width' : '._io.terminalwriter:get_terminal_width',| @@ -0,0 +1,94 @@ | |||
| import sys | |||
There was a problem hiding this comment.
For py.path, we should only expose class local. LocalPath is not importable directly, only as the py.path.local. And the SVN stuff I guess is dead and better to leave out.
Should remove everything else from this file and py/path/common.pyi or expose underscored if referenced indirectly by an exposed item.
|
BTW, I just checked the Might help direct the focus. |
stubgen is great for this, but I think we should take its output, trim it down and restructure it, as I suggested above, and later also fill up the (I can help with the effort if you'd like) |
Sure, of course. Your help would be appreciated of course. |
Great, I can do it now. I'll just send a PR with my suggestions against your repo so you can decide what to do. |
|
Closing in favor of #232. |
2 participants
This is meant to be used for pytest, which mainly uses
py.path.local, and is very much work in progress./cc @bluetech