Skip to content

Improve external quoting logic#3579

Merged
sophiajt merged 4 commits intonushell:mainfrom
sophiajt:improve_external_quoting
Jun 8, 2021
Merged

Improve external quoting logic#3579
sophiajt merged 4 commits intonushell:mainfrom
sophiajt:improve_external_quoting

Conversation

@sophiajt
Copy link
Copy Markdown
Contributor

@sophiajt sophiajt commented Jun 8, 2021

This PR:

  • Improves the quoting logic for external arguments
  • Adds a new testbin called "meow" which does a cat
  • Adds a bit of additional escaping logic

fixes #3329

@sophiajt sophiajt merged commit a021b99 into nushell:main Jun 8, 2021
@sophiajt sophiajt deleted the improve_external_quoting branch June 8, 2021 21:00
fn external_arg_with_quotes() {
Playground::setup("external_arg_with_quotes", |dirs, sandbox| {
sandbox.with_files(vec![FileWithContent(
"sample.toml",
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jonathandturner, I'm wondering what happens if the path contains spaces? Or do you tackle this case in another issue?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Paths with spaces should work. I've done a few completion updates, which includes improving how we handle paths with spaces.

Are you seeing any issues with the latest PRs?

Copy link
Copy Markdown
Contributor

@schrieveslaach schrieveslaach Jun 9, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My thought was that the test could reflect that spaces are supported. Like that:

Suggested change
"sample.toml",
"a sample file.toml",

So, I don't see any trouble, just a slight test code improvement.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@schrieveslaach if you're interested, we'd love to have more tests to help cover cases like that.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll take a look. 👍

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jonathandturner, I created #3596, do you mind and have a look at it?

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.

Tilde in Quotes With External Command Not Working

2 participants