Skip to content

Ensures that non-strings are not unnecessarily quoted#5492

Closed
merelymyself wants to merge 7 commits intonushell:mainfrom
merelymyself:main
Closed

Ensures that non-strings are not unnecessarily quoted#5492
merelymyself wants to merge 7 commits intonushell:mainfrom
merelymyself:main

Conversation

@merelymyself
Copy link
Copy Markdown
Contributor

@merelymyself merelymyself commented May 9, 2022

Description

Intended to resolve issue #5472. By performing rigorous check that a string needs to be escaped.

The primary issue comes with negative numbers.

Tests

Make sure you've run and fixed any issues with these commands:

  • cargo fmt --all -- --check to check standard code formatting (cargo fmt --all applies these changes)
  • cargo clippy --all --all-features -- -D warnings -D clippy::unwrap_used -A clippy::needless_collect to check that you're using the standard code style
  • cargo build; cargo test --all --all-features to check that all the tests pass

@merelymyself merelymyself marked this pull request as draft May 10, 2022 06:25
@merelymyself
Copy link
Copy Markdown
Contributor Author

@uasi , I think this should fix it - could you take a look? It collects the types and if it is 'math', 'number' or 'int' it sends the negative number on with quotes.

@uasi
Copy link
Copy Markdown
Contributor

uasi commented May 10, 2022

@merelymyself Thank you for the hard work, but I've come to think that simply making quoting smarter does not solve the overall problem (see #5472 (comment) ). It appears it requires the argument evaluation semantics need to be redesigned at a deeper level of the language. Please contact one of the core devs, as I'm just a new-ish contributor.

@merelymyself merelymyself marked this pull request as ready for review May 10, 2022 12:22
@merelymyself
Copy link
Copy Markdown
Contributor Author

@uasi I honestly think you might be right, it may be better in the long-run to redesign the argument evaluation semantics. The whole of deparse.rs feels like a band-aid. That said, I think it may be better to keep the bandaid on for the moment. I suggest you open an issue on this.

@sophiajt
Copy link
Copy Markdown
Contributor

I also think I agree with uasi. The problem with the argument parsing we do now is that it feels very much "solve one case but unfortunately break another case". With a rewrite, we can start by making sure we have all the cases we want to cover first, and then make sure the parser and the parts that deal with args all understand it.

@fdncred
Copy link
Copy Markdown
Contributor

fdncred commented May 11, 2022

how do we move forward on this? should we close this PR and talk about it in design-discussion or land this and continue to try and make it better?

@merelymyself
Copy link
Copy Markdown
Contributor Author

closing this for the moment.

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.

4 participants