Conversation
ap--
left a comment
There was a problem hiding this comment.
Thank you so much for the PR! 🙏 ❤️
I made a few comments. Let me know what you think.
Cheers,
Andreas
|
|
||
| def test_relative_to(self): | ||
| assert 'file.txt' == UPath("file:///test_bucket/file.txt").relative_to(UPath("file:///test_bucket")).path | ||
|
|
There was a problem hiding this comment.
It would be great if you could add a test that checks if a ValueError is raised if a UPath of a different protocol is provided as argument.
And a test that checks the cases for the walk_up keyword argument.
There was a problem hiding this comment.
added a test for s3, not sure what walk_up allows will need more time to look at that to write a test case
There was a problem hiding this comment.
added a test for walk_up seems like it's broken for azure
There was a problem hiding this comment.
seemingly its because UPath("az:///other_test_bucket").parents is an empty list which I don't understand
|
You can prevent some of the failing tests, by editing Lines 22 to 28 in 380144c to use: This is due to a incompatibility of UPath's test suite with the newest fsspec. |
|
Where are we with this? I would be nice to have this in, because the current behavior is broken :-). |
|
With the new fsspec release, this is ready to be merged. I'm on holiday until the end of May and will prepare a new release when I'm back 😊 |
|
Great! |
|
Thinking and testing this a little bit further (from an Airflow perspective, but I don't think that matters). The following does not seem to make sense: prints which doesn't make sense either way. If posix it should resolve to Isn't it better to return a PurePath or keep a reference to the original? |
|
What I am considering is to have
|
|
coming back to clean up some old PRs, completely no idea what I was attempting to do here as was for an old job but have merged the recent changes and rebased. Let me know if I can help |
|
Would be nice to have this resolved. I agree that returning PosixUPath is not ideal, but at least its better than the current state. |
|
Closing in favour of #405. Similar to what @bolkedebruin suggested, but currently with a private base path. Could think about exposing it via a special storage option as suggested. |
Close #214
Test that should pass if the local path code worked as expected