Add stubs for typing #232

blueyed · 2020-01-19T10:55:32Z

This is meant to be used for pytest, which mainly uses py.path.local, and is very much work in progress.

Continuation of #231

bluetech

Will be nice to squash to one commit.

Also, as far as I'm concerned this is pretty much done, not WIP :) Unless there is something more you want to add?

blueyed · 2020-01-19T11:47:22Z

Will be nice to squash to one commit.

Sure. Would be nice to do it via GitHub's UI, but it's disabled here also.
I think keeping the history here is useful in general, so we should use yet another PR then for the squashed result (or merge it manually).

As for "WIP": I think it's good to use it via this/your branch for a while before merging.

bluetech · 2020-01-19T13:11:11Z

As for "WIP": I think it's good to use it via this/your branch for a while before merging.

Do you mean use it in pytest or something else? If pytest, then there is not much using the stubs beyond running mypy on the code with the stubs installed, and we did do that.

Are there any other projects using py + mypy that you know about? I can check them as well.

blueyed · 2020-01-19T13:52:58Z

As for "WIP": I think it's good to use it via this/your branch for a while before merging.

Do you mean use it in pytest or something else? If pytest, then there is not much using the stubs beyond running mypy on the code with the stubs installed, and we did do that.

Yes. But I do not think we need to merge it already, and certainly not until others agree that it should be e.g. shipped.
Having the branch here is good enough for us to help with typing pytest, and could even be used on our CI already via it - so there is no need to rush it.

Are there any other projects using py + mypy that you know about? I can check them as well.

I don't know of any.

bluetech · 2020-01-19T14:07:04Z

Yes. But I do not think we need to merge it already, and certainly not until others agree that it should be e.g. shipped.

I suspect that with a WIP label it won't get attention by others. I think the stubs are complete, and there is not much to test beyond just using it once in pytest, so I just wonder what more you think is needed in order to get the WIP label down.

blueyed · 2020-01-19T18:15:48Z

@bluetech agreed. Not really WIP, but waiting for feedback and should not get merged as-is. I've removed the label.

bluetech · 2020-01-24T11:48:11Z

@blueyed We should add this diff:

diff --git a/py/path.pyi b/py/path.pyi
index acbee979..2556a732 100644
--- a/py/path.pyi
+++ b/py/path.pyi
@@ -49,7 +49,7 @@ if sys.version_info >= (3, 6):
 else:
     class _PathLike(Generic[AnyStr]):
         def __fspath__(self) -> AnyStr: ...
-_PathType = Union[bytes, Text, _PathLike[Any]]
+_PathType = Union[bytes, Text, _PathLike[Any], local]
 
 class local:
     class ImportMismatchError(ImportError): ...

While I thought py.path.local would just be covered by the os.PathLike, it's not the case currently (python/typeshed#2808).

blueyed · 2020-01-24T19:09:29Z

@bluetech ok, please push it here then - I cannot really tell/help in that regard.

bluetech · 2020-01-25T11:43:57Z

Huh, didn't notice this PR is now based on my branch.

I pushed this and another fix.

blueyed · 2020-01-27T20:26:01Z

@bluetech please git push bluetech 1.8.1 (i.e. the latest tag to your repo), so that setuptools-scm gets the correct/proper version.

blueyed · 2020-01-27T20:26:27Z

You might also want to fix the conflict with appveyor.yml, so that CI runs.
Could/should also revert 66fc1bf then - I'll go ahead and do that.

blueyed · 2020-01-27T20:29:00Z

@bluetech re the above: I do not have permission to push to this branch also (allow it via checkbox on the right?). But would still not allow me to push tags probably.

bluetech · 2020-01-27T22:57:27Z

@blueyed done.

Do you think there is anything else blocking this PR? Should we ask anybody for additional review or +1s? py.path.local shows up everywhere in pytest, so it'd be really nice to have it :)

blueyed · 2020-01-27T23:03:17Z

Do you think there is anything else blocking this PR?

Not really, apart from that we/you are still added fixes/improvements to it.

Should we ask anybody for additional review or +1s?

Sure.

py.path.local shows up everywhere in pytest, so it'd be really nice to have it :)

I'm going to add this branch to the "mypy" tox env, which then can be run on CI already. Having the version (tag) for it was a requirement.

bluetech · 2020-01-27T23:15:24Z

Okay, pinging some names I recognize from the contributor's list.

@nicoddemus @RonnyPfannschmidt @asottile

This PR adds type stubs to py. The stubs only cover the modules that pytest uses, but are comprehensive and strict, otherwise. We tested pytest's typing with typed py installed and it works well. The py.typed file indicates the stubs are "partial", so projects which use modules which we didn't cover shouldn't be badly affected.

Would be great to have your +1's, unless you think this is not a good idea.

asottile · 2020-01-27T23:34:18Z

the stubs seem fine, though for a library which is "maintenance only" I think this extends a bit beyond that -- that said it does improve pytest's ability to typecheck itself so I'm a little torn 🤔

blueyed · 2020-01-28T17:07:37Z

py/path.pyi

+        def __fspath__(self) -> AnyStr: ...
+_PathType = Union[bytes, Text, _PathLike[Any], local]
+
+class local:


Suggested change

class local:

class local(_PathLike[str]):

This would allow for pathlib.Path(py.path.local("foo")) then, where mypy currently complains:

Argument 1 to "Path" has incompatible type "local"; expected "Union[str, _PathLike[str]]" [arg-type]

Note that it should only accept Text in general (#238).

This would allow for pathlib.Path(py.path.local("foo")) then, where mypy currently complains:

The situation is odd. If we also do it for the real local (in the .py, not just in the .pyi) then that would remove any doubt from my mind.

For now a workaround for pathlib.Path(py.path.local("foo")) is pathlib.Path(str(py.path.local("foo"))). This is also be portable to Python<=3.5.

Yeah, the workaround is known, but currently mypy complains about something that works (with py36+).
We can leave it out for now if unclear, but should probably make it more strict to not accepts bytes?

We can leave it out for now if unclear

If we make the real local derive from os.PathLike, then it would be good to add it in the stubs too. But if not, then the stub would be sort-of lying. Although os.PathLike has a hack where isinstance(foo, os.PathLike) returns true even if foo just implements __fspath__, without deriving. So I'm not really sure.

but should probably make it more strict to not accepts bytes?

While it seems bytes support is broken, I'm sure there's someone someplace that uses it. For example #187 (comment). So again, not sure :)

An article on how pathlib (does not) handle this: https://jodal.no/2019/12/10/pathlib-and-paths-with-arbitrary-bytes/

blueyed · 2020-01-28T21:16:07Z

commit cae97e28 (HEAD -> pyi)
Author: Daniel Hahler <git@thequod.de>
Date:   Tue Jan 28 22:14:25 2020 +0100

    path: __truediv__, __div__, join: returns local

diff --git a/py/path.pyi b/py/path.pyi
index b7879c3e..e803ada2 100644
--- a/py/path.pyi
+++ b/py/path.pyi
@@ -65,8 +65,8 @@ class local:
     def __gt__(self, other: object) -> bool: ...
     def __add__(self, other: object) -> local: ...
     def __cmp__(self, other: object) -> int: ...
-    def __div__(self, other: _PathType): ...
-    def __truediv__(self, other: _PathType): ...
+    def __div__(self, other: _PathType) -> local: ...
+    def __truediv__(self, other: _PathType) -> local: ...
     def __fspath__(self) -> str: ...

     @classmethod
@@ -132,7 +132,7 @@ class local:
     def isdir(self) -> bool: ...
     def isfile(self) -> bool: ...
     def islink(self) -> bool: ...
-    def join(self, *args: _PathType, abs: int = ...) -> Any: ...
+    def join(self, *args: _PathType, abs: int = ...) -> local: ...
     def listdir(
         self,
         fil: Optional[Union[str, Text, Callable[[local], bool]]] = ...,

codecov-io · 2020-01-28T21:43:35Z

Codecov Report

Merging #232 into master will not change coverage by %.
The diff coverage is 66.66%.

@@           Coverage Diff           @@
##           master     #232   +/-   ##
=======================================
  Coverage   82.06%   82.06%           
=======================================
  Files          55       55           
  Lines       10161    10161           
  Branches     1141     1141           
=======================================
  Hits         8339     8339           
  Misses       1558     1558           
  Partials      264      264

blueyed · 2020-02-01T22:44:40Z

@bluetech seems you've not really applied the patch? (2cf11b7 is empty)
You can cherry-pick it via cae97e2.

I guess you could also invite me as collaborator to your fork.

bluetech · 2020-02-10T08:02:59Z

@blueyed Oops. Applied now. I also went and squashed it. I know you asked not to, but it was pretty terrible! I put the old history here: https://github.com/bluetech/py/tree/pyi-old

Fixes: > Argument 1 to "Path" has incompatible type "Union[local, Any]"; > expected "Union[str, _PathLike[str]]" [arg-type] Ref: pytest-dev/py#232 (review)

py/iniconfig.pyi

bluetech · 2020-02-28T11:38:22Z

@asottile

the stubs seem fine, though for a library which is "maintenance only" I think this extends a bit beyond that -- that said it does improve pytest's ability to typecheck itself so I'm a little torn

Since this doesn't make any code changes, I think the risk is quite low. But I understand the hesitation.

To move this forward, maybe we can submit it to typeshed instead. @blueyed, WDYT?

blueyed · 2020-03-01T19:49:12Z

@bluetech
From my point of view it works for me by using this branch locally and for CI.
I do not think it is "worth" adding to typeshed, given that only pytest uses it probably.
I'm not opposed to merging and properly releasing it though, of course - but cannot really tell if that implies going through extra things when updating/fixing them.

bluetech · 2020-03-05T11:59:54Z

I see 3 options to proceed:

Use py from a branch. This doesn't work for users, or official pytest CI. I think it's better to avoid this, to allow everyone to benefit.
Merge this PR and release py. This is the best outcome IMO, but seems to have maintenance concerns and py is officially frozen. So probably not going to happen?
Add to typeshed. I can manage and maintain it myself. Doesn't incur a maintenance burden on py itself. Typeshed is open to 3rd party stubs.

That's why going through typeshed seems like the best course to me for now. But of course I wouldn't submit it without consensus.

RonnyPfannschmidt · 2020-03-05T13:41:03Z

im ok with option 2 in a feature release - this is clearly a help for any downstream that wants to adopt typing

bluetech · 2020-03-29T16:26:32Z

Added a few improvements to py.xml while adding annotations to _pytest.junitxml.

bluetech · 2020-03-29T16:36:54Z

... and squashed again.

@RonnyPfannschmidt said:

im ok with option 2 in a feature release - this is clearly a help for any downstream that wants to adopt typing

Thanks! So I guess we have +1 from @blueyed, @RonnyPfannschmidt (and me), and a -0 (?) from @asottile.

If anyone with permissions feels like merging this (and maybe releasing), that would be great.

If there will be any issues reported after releasing this, I'll make sure to handle them (I subscribed to issues on this repo).

bluetech · 2020-06-22T08:40:36Z

There were a couple of omissions in the py.xml types, I fixed them:

Didn't accept py.xml.raw where it's allowed
Didn't accept Iterable of Tags where it's allowed

nicoddemus

LGTM, should we update the CHANGELOG?

Add complete type stubs to the public interface of some of `py`s modules. The modules are those used by `pytest`. The types are marked as partial, so uncovered modules should be unaffected. Co-authored-by: Ran Benita <ran@unusedvar.com>.

bluetech · 2020-06-23T16:49:15Z

OK, I made a few updates:

Added CHANGELOG entry (marked for a 1.9.0)
Somewhat belatedly I understood that partial in py.typed doesn't do what I thought it does, so I removed it and added explicit Anys for all of the types & modules we didn't cover.
Made py.path.local explicitly inherit from os.PathLike because it is actually not defined as a Protocol in typeshed. This fixes pathlib.Path(py.path.local()) (at least on Python>=3.6).

Finally, pytest master is currently completely clean with this PR.

So I think this is ready.

nicoddemus · 2020-06-24T22:42:38Z

Awesome! Let's merge and release this then!

This was referenced Jan 19, 2020

[WIP] add stubs for typing #231

Closed

Drop support for EOL Python 3.4 #226

Merged

bluetech approved these changes Jan 19, 2020

View reviewed changes

bluetech mentioned this pull request Jan 19, 2020

Fix type errors after adding types to the py dependency pytest-dev/pytest#6511

Merged

blueyed changed the title ~~[WIP] add stubs for typing~~ Add stubs for typing Jan 19, 2020

This comment has been minimized.

Sign in to view

blueyed commented Jan 28, 2020

View reviewed changes

pytest-dev deleted a comment from codecov-io Jan 28, 2020

bluetech force-pushed the pyi branch from 2cf11b7 to fb3379f Compare February 10, 2020 08:00

bluetech force-pushed the pyi branch from fb3379f to 2b6bd29 Compare February 10, 2020 08:04

blueyed mentioned this pull request Feb 20, 2020

cacheprovider: fix typing with Path(py.path.local) pytest-dev/pytest#6774

Merged

blueyed commented Feb 25, 2020

View reviewed changes

py/iniconfig.pyi Show resolved Hide resolved

bluetech force-pushed the pyi branch from 7996387 to 00c0985 Compare March 29, 2020 16:27

bluetech mentioned this pull request Apr 11, 2020

Support for static typing pytest-dev/pytest#3342

Closed

bluetech mentioned this pull request Jun 12, 2020

#7336 make --log-file relative to inifile if it exists pytest-dev/pytest#7350

Closed

bluetech force-pushed the pyi branch from 00c0985 to 11da4f5 Compare June 22, 2020 08:38

nicoddemus approved these changes Jun 22, 2020

View reviewed changes

bluetech mentioned this pull request Jun 22, 2020

config: improve typing pytest-dev/pytest#7401

Merged

Add type stubs

59fd0d5

Add complete type stubs to the public interface of some of `py`s modules. The modules are those used by `pytest`. The types are marked as partial, so uncovered modules should be unaffected. Co-authored-by: Ran Benita <ran@unusedvar.com>.

bluetech force-pushed the pyi branch from 11da4f5 to 59fd0d5 Compare June 23, 2020 16:42

nicoddemus merged commit c869d32 into pytest-dev:master Jun 24, 2020

bluetech deleted the pyi branch December 12, 2020 11:54

Add stubs for typing #232

Add stubs for typing #232

Conversation

blueyed commented Jan 19, 2020

bluetech left a comment

Choose a reason for hiding this comment

blueyed commented Jan 19, 2020

bluetech commented Jan 19, 2020

blueyed commented Jan 19, 2020

bluetech commented Jan 19, 2020

blueyed commented Jan 19, 2020

bluetech commented Jan 24, 2020

blueyed commented Jan 24, 2020

bluetech commented Jan 25, 2020

blueyed commented Jan 27, 2020

blueyed commented Jan 27, 2020 • edited Loading

blueyed commented Jan 27, 2020

bluetech commented Jan 27, 2020

blueyed commented Jan 27, 2020

bluetech commented Jan 27, 2020

asottile commented Jan 27, 2020

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

blueyed Jan 28, 2020

Choose a reason for hiding this comment

blueyed Jan 28, 2020

Choose a reason for hiding this comment

bluetech Jan 28, 2020

Choose a reason for hiding this comment

blueyed Jan 28, 2020

Choose a reason for hiding this comment

bluetech Jan 28, 2020

Choose a reason for hiding this comment

bluetech Jan 28, 2020

Choose a reason for hiding this comment

blueyed commented Jan 28, 2020

codecov-io commented Jan 28, 2020 • edited Loading

Codecov Report

blueyed commented Feb 1, 2020 • edited Loading

bluetech commented Feb 10, 2020

bluetech commented Feb 28, 2020

blueyed commented Mar 1, 2020

bluetech commented Mar 5, 2020

RonnyPfannschmidt commented Mar 5, 2020

bluetech commented Mar 29, 2020

bluetech commented Mar 29, 2020

bluetech commented Jun 22, 2020 • edited Loading

nicoddemus left a comment

Choose a reason for hiding this comment

bluetech commented Jun 23, 2020

nicoddemus commented Jun 24, 2020

blueyed commented Jan 27, 2020 •

edited

Loading

codecov-io commented Jan 28, 2020 •

edited

Loading

blueyed commented Feb 1, 2020 •

edited

Loading

bluetech commented Jun 22, 2020 •

edited

Loading