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

Revistting tmt's Path usage #3283

Open
LecrisUT opened this issue Oct 11, 2024 · 4 comments
Open

Revistting tmt's Path usage #3283

LecrisUT opened this issue Oct 11, 2024 · 4 comments

Comments

@LecrisUT
Copy link
Contributor

Could we track why we need a custom Path object? Reading the docstring, isn't pathlib.Path.relative_to(walk_up=True) what is needed there?

@happz
Copy link
Collaborator

happz commented Oct 11, 2024

Could we track why we need a custom Path object? Reading the docstring, isn't pathlib.Path.relative_to(walk_up=True) what is needed there?

It is. But that was added in Python 3.12.

To me, it's a matter of convenience. E.g. https://github.com/teemtee/tmt/pull/3263/files#diff-80e5ba103da9529bd832a9cb0ea2712ceeee94b3dd5f92fcc75ed492817366b7R48. I really don't understand the reasoning behind not adding support for "append" to write_text/write_bytes, or even dedicated methods. Python developers decided "who needs this can use open() + mode='a', it's easy" - yeah, it is, but suddenly, I must use open() that's no longer needed for anything, to do something that could have been easily provided by Path classes, just like writing and reading of text blobs... Anyway.

I guess the question is, where does it stand in your way, and why would you like to get rid of it? Maybe we can find some solution.

@LecrisUT
Copy link
Contributor Author

Primarily the issue is when iteracting with other modules that generate a pathlib.Path, e.g. importlib.resources. If at any point in the path construction chain there is a different pathlib.Path or tmt.Path, it will inevitable propagate outwards which is too fragile.

@happz
Copy link
Collaborator

happz commented Oct 14, 2024

Primarily the issue is when iteracting with other modules that generate a pathlib.Path, e.g. importlib.resources. If at any point in the path construction chain there is a different pathlib.Path or tmt.Path, it will inevitable propagate outwards which is too fragile.

Fair enough. It's really a shame there is no pathlib factory we could tell to use our class throughout the whole standard library.

If we decide to drop the class, we would need to replace its methods, probably with some tmt.utils helpers. It will be a shame though, I feel that WRT readability, returning to relative_to(some_path) or unrooted(some_path) is much worse than some_path.unrooted() :/ But it's doable, sure, the class exists only to 1. "fix" relative_to() - why the hell they dropped the walk_up functionality just to add it after several releases :/ - and 2. for convenience, like append_text() - again, why, just why forcing users to use open() just for one kind of writing into a file...

@LecrisUT
Copy link
Contributor Author

Yeah, the OOP interface is much neater, and backports for stdlib can be quite tricky, since you cannot rely on isinstance since you have to check for both backport and stdlib variants.

In principle the current approach could still work, but we need to be confident that mypy and pyright are catching all type modifications and we need to be careful not to loosely type-hint, e.g. in #2947 we need to change it from Traversable to strictly tmt.Path | MultiplexedPath.

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

No branches or pull requests

2 participants