Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Provide conversion to and from pathlib.Path in anyio.Path #844

Open
1 task done
sanmai-NL opened this issue Dec 18, 2024 · 22 comments
Open
1 task done

Provide conversion to and from pathlib.Path in anyio.Path #844

sanmai-NL opened this issue Dec 18, 2024 · 22 comments
Labels
enhancement New feature or request

Comments

@sanmai-NL
Copy link

Things to check first

  • I have searched the existing issues and didn't find my feature already requested there

Feature description

There's a private attribute _path already.

Use case

Can you please expose it, so that APIs expecting pathlib.Path objects can use them. Currently, I convert anyio.Path objects to strings since it's most succinct, but that detracts from type safety and code base conventions.

@sanmai-NL sanmai-NL added the enhancement New feature or request label Dec 18, 2024
@agronholm
Copy link
Owner

anyio.Path works with whatever APIs consumer PathLike objects (which is pretty much all the stdlib APIs dealing with paths). If an API was badly designed to only accept actual pathlib.Path objects, then you can just convert them directly as pathlib.Path(your_anyio_path_obj).

@sanmai-NL
Copy link
Author

Type checkers like Mypy disagree though.

@agronholm
Copy link
Owner

What API gives such errors then?

@agronholm
Copy link
Owner

Are you getting errors at run time?

@sanmai-NL
Copy link
Author

sanmai-NL commented Dec 18, 2024

On your point of API compatibility, unexpectedly passing anyio.Path objects to non-anyio-API functions may lead to runtime issues. These types are not substitutes, as many methods on anyio.Path objects require an async context and need to be awaited, leading to stalls and missed branches (e.g., if not path.exists(): behaves differently).

Coercing the objects in the way you suggest results in type checking faults (under Mypy 1.13.0).

@agronholm
Copy link
Owner

You haven't answered either one of my questions yet.

@sanmai-NL
Copy link
Author

@agronholm

What API gives such errors then?

Any API could, for example:

from anyio import Path as PathAnyio
from pathlib import Path
from tarfile import open as tarfile_open


def test_std(*, path_std: Path) -> None:
    assert not path_std.exists()
    with tarfile_open(name=path_std):
        pass


def test_anyio(*, path_anyio: PathAnyio) -> None:
    # Passes type check, but is incorrect since this path isn't awaited. https://github.com/python/mypy/issues/16069.
    # One reason to avoid accepting either `Path` and `PathAnyio` as a single argument.
    assert not path_anyio.exists()
    # Works.
    with tarfile_open(name=path_anyio):
        pass


def main() -> None:
    path_std = Path("test.tar")
    path_anyio = PathAnyio("test.tar")
    test_anyio(path_anyio=path_anyio)
    test_anyio(path_anyio=path_std)
    test_std(path_std=path_std)
    test_std(path_std=path_anyio)
    test_std(path_std=Path(path_anyio))


if __name__ == "__main__":
    main()
test.py: note: In function "main":
test.py:25: error: Argument "path_anyio" to "test_anyio" has incompatible type "pathlib.Path"; expected "anyio._core._fileio.Path"  [arg-type]
test.py:27: error: Argument "path_std" to "test_std" has incompatible type "anyio._core._fileio.Path"; expected "pathlib.Path"  [arg-type]
Found 2 errors in 1 file (checked 1 source file)

Can you explain what you find badly designed about an API that doesn't conflate anyio.Path and pathlib.Path objects in some kind of duck typing sense? (They aren't even duck typing substitutable, since the async context doesn't allow for that with a substantial share of the methods).

Are you getting errors at run time?

I wrote this:

Can you please expose it, so that APIs expecting pathlib.Path objects can use them. Currently, I convert anyio.Path objects to strings since it's most succinct, but that detracts from type safety and code base conventions.

I don't see the relation with run-time you refer to. Mypy is a type checker, that analyses statically, i.e., not at run-time.

@agronholm
Copy link
Owner

agronholm commented Dec 18, 2024

Can you explain what you find badly designed about an API that doesn't conflate anyio.Path and pathlib.Path objects in some kind of duck typing sense?

Not duck typing sense. Stdlib APIs, for example, are defined in typeshed to accept path-like objects (os.PathLike) or str, not pathlib.Path, since accepting only pathlib.Path would unnecessarily narrow the usable argument types. They then proceed to convert the argument to using path = Path(arg) which will convert an AnyIO Path just fine. Do you have a problem with this approach?

I don't see the relation with run-time you refer to. Mypy is a type checker, that analyses statically, i.e., not at run-time.

I'm trying to understand the extent of the problem you have with this (to see if it's a mypy-only issue).

@agronholm
Copy link
Owner

This is what I'm talking about passes mypy in strict mode):

import asyncio.tasks
from pathlib import Path

import anyio


async def test_func(p: Path) -> None:
    pass


if __name__ == "__main__":
    path = anyio.Path("/foo")
    asyncio.run(test_func(Path(path)))

@sanmai-NL
Copy link
Author

sanmai-NL commented Dec 18, 2024

First, about the actual solution you proposed. It's nice that Path(path_anyio) works, I didn't know that. We have a practical problem with that though: it requires us to from pathlib import Path in every module where we do this, leading to a name collision. A to_path() method or path field would let us avoid that.

Then about the argument signature. We don't want to accept str paths, since we consider using strings as Paths a bad practice. I've been looking into os.PathLike but it's unclear to me how to constrain that to pathlib plus anyio path-likes but not strings.

@agronholm
Copy link
Owner

I've been looking into os.PathLike but it's unclear to me how to constrain that to pathlib plus anyio path-likes but not strings.

If you annotate a parameter as os.PathLike[str], it accepts both but not str.

@agronholm
Copy link
Owner

First, about the actual solution you proposed. It's nice that Path(path_anyio) works, I didn't know that. We have a practical problem with that though: it requires us to from pathlib import Path in every module where we do this, leading to a name collision. A to_path() method or path field would let us avoid that.

That anyio.Path has its own internal Path is just an implementation detail which I don't necessarily want to expose.

@sanmai-NL
Copy link
Author

I've been looking into os.PathLike but it's unclear to me how to constrain that to pathlib plus anyio path-likes but not strings.

If you annotate a parameter as os.PathLike[str], it accepts both but not str.

That's a nice tidbit of documentation I would say.

For the function signatures I find in our present product, I still don't want to use this since there's the risk of not checking for and handling the particulars of each class.

@sanmai-NL
Copy link
Author

First, about the actual solution you proposed. It's nice that Path(path_anyio) works, I didn't know that. We have a practical problem with that though: it requires us to from pathlib import Path in every module where we do this, leading to a name collision. A to_path() method or path field would let us avoid that.

That anyio.Path has its own internal Path is just an implementation detail which I don't necessarily want to expose.

Yeah, and that's why I opened this issue. To discuss this use case. A to_path() method would still be fine then, no?

@agronholm
Copy link
Owner

First, about the actual solution you proposed. It's nice that Path(path_anyio) works, I didn't know that. We have a practical problem with that though: it requires us to from pathlib import Path in every module where we do this, leading to a name collision. A to_path() method or path field would let us avoid that.

That anyio.Path has its own internal Path is just an implementation detail which I don't necessarily want to expose.

Yeah, and that's why I opened this issue. To discuss this use case. A to_path() method would still be fine then, no?

I'm not inclined to add any extra code without good reason. And not wanting to import something is definitely not a good reason. It doesn't increase memory footprint given that AnyIO already imports it. What's stopping you from doing import pathlib and then func(pathlib.Path(path_anyio)), in case you don't want to support general PathLikes?

@agronholm
Copy link
Owner

I least write all my path-accepting APIs elsewhere to allow any os.PathLike, and implicitly wrap the argument in pathlib.Path() (or anyio.Path()).

@sanmai-NL
Copy link
Author

First, about the actual solution you proposed. It's nice that Path(path_anyio) works, I didn't know that. We have a practical problem with that though: it requires us to from pathlib import Path in every module where we do this, leading to a name collision. A to_path() method or path field would let us avoid that.

That anyio.Path has its own internal Path is just an implementation detail which I don't necessarily want to expose.

Yeah, and that's why I opened this issue. To discuss this use case. A to_path() method would still be fine then, no?

I'm not inclined to add any extra code without good reason. And not wanting to import something is definitely not a good reason. It doesn't increase memory footprint given that AnyIO already imports it. What's stopping you from doing import pathlib and then func(pathlib.Path(path_anyio)), in case you don't want to support general PathLikes?

We do import, of course, otherwise conversion wouldn't work. We favor from imports over basic imports for reasons of brevity. For this particular case, we could indeed use this workaround nonetheless.

@agronholm
Copy link
Owner

First, about the actual solution you proposed. It's nice that Path(path_anyio) works, I didn't know that. We have a practical problem with that though: it requires us to from pathlib import Path in every module where we do this, leading to a name collision. A to_path() method or path field would let us avoid that.

That anyio.Path has its own internal Path is just an implementation detail which I don't necessarily want to expose.

Yeah, and that's why I opened this issue. To discuss this use case. A to_path() method would still be fine then, no?

I'm not inclined to add any extra code without good reason. And not wanting to import something is definitely not a good reason. It doesn't increase memory footprint given that AnyIO already imports it. What's stopping you from doing import pathlib and then func(pathlib.Path(path_anyio)), in case you don't want to support general PathLikes?

We do import, of course, otherwise conversion wouldn't work. We favor from imports over basic imports for reasons of brevity. For this particular case, we could indeed use this workaround nonetheless.

How about aliasing the import then, like how you did it in your original post?

@sanmai-NL
Copy link
Author

Yes, that's what we do now. The need to import the clas(ses) across many files to convert is less ergonomical than a to_path() would be (my earlier remark). I'm curious: what would the cost be to you to add this method?

@agronholm
Copy link
Owner

Yes, that's what we do now. The need to import the clas(ses) across many files to convert is less ergonomical than a to_path() would be (my earlier remark). I'm curious: what would the cost be to you to add this method?

There is little "cost" per se - I just don't want to encourage the wrong kinds (IMHO) of design patterns.

@sanmai-NL
Copy link
Author

Speaking of which: is making anyio.Path a PathLike the best possible design? Do you have some standard library examples where supplying either pathlike object will work correctly? I've encountered many instances where an awaitable operation is performed on it, but the standard library is fully unaware of it.

@agronholm
Copy link
Owner

Speaking of which: is making anyio.Path a PathLike the best possible design?

What's the alternative then? Being PathLike just means that it has the __fspath__() method which spits out the path as a string, so it can be easily converted to a pathlib.Path.

Do you have some standard library examples where supplying either pathlike object will work correctly?

  • open() from builtins
  • zipfile.ZipFile
  • os.stat()
  • toml.decoder.load()

I've encountered many instances where an awaitable operation is performed on it, but the standard library is fully unaware of it.

That sentence made no sense to me. What awaitable operation is performed on what, by what code?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

2 participants