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.

Sign up for GitHub

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

Jump to bottom

path-escape-flag: add escape flag, replace backslashes with double ba… #9848

Closed
wants to merge 4 commits into from
Closed

path-escape-flag: add escape flag, replace backslashes with double ba… #9848

wants to merge 4 commits into from

Conversation

atahabaki
Copy link
Contributor

@atahabaki atahabaki commented Jul 29, 2023 •
edited

Related issue: #9838

Changes:

  • added --escape flag to path join subcommand.
  • replaces \ with \\.
  • Just made it available to Windows users only.

@atahabaki atahabaki mentioned this pull request Jul 29, 2023
Closed
@amtoine
Copy link
Member

amtoine commented Jul 29, 2023

@atahabaki
should this close #9838? 😋

@atahabaki
Copy link
Contributor Author

@atahabaki
should this close #9838? 😋

Yeah, it should close.

@amtoine amtoine linked an issue Jul 29, 2023 that may be closed by this pull request
str expand doesn't like backslashes for path separators #9838
Closed
sholderbach
sholderbach requested changes Jul 29, 2023
View reviewed changes
crates/nu-command/src/path/join.rs Outdated Show resolved Hide resolved
@fdncred
Copy link
Collaborator

fdncred commented Jul 29, 2023

Seems like we could have these changes without duplicating everything for windows or unix.

But the real question here is whether it's better for Windows users to have ... | path join --escape available as a QOL improvement versus having to do ... | path join | str replace '\\' '\\\\' -a when needed. This is also being discussed here #9838.

My vote is to have --escape unless we can come up with another command that understand when it's appropriate to escape things.

@atahabaki atahabaki marked this pull request as ready for review July 29, 2023 14:20
@fdncred
Copy link
Collaborator

fdncred commented Jul 31, 2023

I originally thought this was the best place to add this functionality but let's close this one and land #9856. Thanks for working with us on this!

@fdncred fdncred closed this Jul 31, 2023
@atahabaki atahabaki deleted the path-escape-feature branch August 2, 2023 16:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sholderbach sholderbach Awaiting requested review from sholderbach

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

str expand doesn't like backslashes for path separators
4 participants
@atahabaki @amtoine @fdncred @sholderbach