Skip to content

Allow path relative-to to use parent directory symbols #14055

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

Open
wants to merge 7 commits into
base: main
Choose a base branch
from

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