Update open process types #3076
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #3076 +/- ##
=======================================
Coverage 99.60% 99.60%
=======================================
Files 121 121
Lines 17882 17882
Branches 3214 3214
=======================================
Hits 17811 17811
Misses 50 50
Partials 21 21
|
|
The updated documentation makes no mention of |
|
I figured it's fine to have more accurate documentation (type hint) and less accurate but clearer (what's written in the docstring). Both are visible in the built docs. I can make them the same though. |
|
Huh https://github.com/python-trio/trio/actions/runs/10649671109/job/29520147803#step:4:208 shouldn't be happening. OK it's because Something I noticed while investigating caching is I'm not sure it's helping much. In fact I think we'd probably be better off without it -- the cache key seems to change every single commit which makes no sense!! |
A5rocks
force-pushed
the
update-open-process-types
branch
from
9796012 to
ede658f
Compare
|
The cache has been working a few days ago, this run for example. We'd definitely want it if possible. Could the problem be that the Cython build isn't using |
It wasn't Cython, it was PyPy on Windows (specifically). It should be using the Here's my current thoughts: the action should only cache pip's cache directory: so there must be something bad there. However, as this run shows, it does actually look at the current Also, I'm struggling to understand why the "Setup python" step has a different hash in the cache key than the "Post Setup python" step in this run: https://github.com/python-trio/trio/actions/runs/10649416223/job/29519569640. (which I reran for debug logging and which... failed???) |
A5rocks
force-pushed
the
update-open-process-types
branch
from
8d36516 to
ede658f
Compare
| @@ -304,7 +304,7 @@ def kill(self) -> None: | |||
|
|
|||
|
|
|||
| async def _open_process( | |||
| command: list[str] | str, | |||
| command: StrOrBytesPath | Sequence[StrOrBytesPath], | |||
There was a problem hiding this comment.
Wondering, why is type var here called StrOrBytesPath? Wouldn't it be StrOrBytes, or is StrOrBytesPath special somehow?
There was a problem hiding this comment.
Because it's typed as str | bytes | PathLike[str] | PathLike[bytes]?
There was a problem hiding this comment.
Reading the docstring currently in place, I wasn't under the impression it supported pathlikes. If you could update that then, that would be great!
There was a problem hiding this comment.
The docstring atm distinguishes what is possible with shell=True and what isn't, but I think paths are fine either way. I can update the docstring though. (this also appears in the function signature Sphinx makes)
There was a problem hiding this comment.
I would seem to be for the best to mention that, yes. After that, I have no objections.
This should resolve the issue mentioned in Gitter