Skip to content

Conversation

@uek-1
Copy link
Contributor

@uek-1 uek-1 commented Oct 11, 2024

Description

Addresses #13969. Allows path relative-to to generate relative paths that use the parent directory .. within them. Also changes the error type to be slightly more informative than the previous one.

User-Facing Changes

Tests + Formatting

Added test example:

  • 'a/b/c' | path relative-to 'a/d/e'

@WindSoilder
Copy link
Contributor

Hi, thank you for your contribution!
Would you please fix the clippy failure and add a test for it?

@uek-1
Copy link
Contributor Author

uek-1 commented Jan 30, 2025

I added a single test example that targets this feature, should I add more?

@uek-1 uek-1 marked this pull request as ready for review February 2, 2025 22:16
@WindSoilder
Copy link
Contributor

Thank you for your contribution! After looking into the pr, and relative issue. I think it's better to make it as a opt-in flag.

Can you please add a flag for the new behavior? I think --walkup is good enough. Then:

> 'a/b/c' | path relative-to 'a/d/e'   # still raise error
> 'a/b/c' | path relative-to 'a/d/e'  --walkup   # returns ../../b/c

@sholderbach
Copy link
Member

@WindSoilder is there a specific concern why Python locks that behind a kwarg? Are there situations where it is problematic in certain ways or was it just added on pythons side to preserve backwards-compatibiliity?

@WindSoilder
Copy link
Contributor

After looking into relative pr: python/cpython#118780
I think historically, relative_to support multiple positional arguments, at that time, walkup is already a keyword only argument. So it just keeps the way to use walkup.

@sholderbach
Copy link
Member

It was added in python/cpython#19813 to satisfy python/cpython#84538 which requested it to be hidden behind a switch.

The behavioral difference was whether symlinks are resolved in the process. Haven't thought through how that may affect us? With the current impl there is no attempt at resolving links or hitting the file system. It just assumes clean Unix paths (do we need to consider Windows specialties here?)

@sholderbach
Copy link
Member

Some other thoughts in neovim/neovim#31790 (comment)

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

Successfully merging this pull request may close these issues.

3 participants