-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
Strings are parsed inconsistently for external commands #12830
Comments
It seems like we need to be consistent, but we also need to be able to tell when to expand a glob or ~. |
To be fair, we don't have to be consistent, but our current implementation feels more like a hack than a proper solution. So this is technically not a bug, just something I want to bring into attention. |
Maybe I don't understand what you're saying because I'd argue that we indeed have to be consistent. If the user is going to opt-in to globbing or expansion, they have to know how to do it and that means it needs to have some level of consistency, otherwise it's a confusing mess. |
The current behavior is consistent for the user (bare-strings/backticks = globbing/expansion). It's only inconsistent for the developer (i.e. the current me), who has to handle strings differently based on where they came from. |
I'll close this for now. Maybe we can revisit this later. |
Describe the bug
When passing string literals to external commands, any surrounding quotes are included in the resulting
Expr::String
. However, when passing a list of string literals using the spread operator, any surrounding quotes are NOT included in the resultingExpr::String
.How to reproduce
dbg!(call)
to therun()
function incrates\nu-command\src\system\run_external.rs
.^ls "foo" ...[ "bar" ]
Here's the output:
Expected behavior
Currently
run_external
gets special treatment so that strings retain their surrounding quotes. The quotes are then trimmed withinrun_external
so it can use the quotes to decide whether to perform glob/tilde expansions or not.I think this is not a good solution, for several reason:
run_external
only needs to know one bit of information: whether the string was evaluated from an unquoted/backtick-quoted string literal. That information can be provided as a boolean.run_external
is not the only command that needs to handle expansions. We should cover the use cases of other commands too, likels
andrm
.My ideal solution would be to change
Value::String
into something like this:Various
eval()
functions should setbare
to true if and only if it was evaluated from an unquoted/backtick-quoted string literal. This way, commands that want to handle expansions can do it without ever touching the syntax tree.While we're at it, let's also remove the
Value::Glob
variant. The proposedValue::String
variant is sufficient to cover all use cases.I'm currently doing a full-rewrite of the
run_external
module to improve readability (Issue #6011). I'll finish the rewrite regardless of whether we change anything or not, but it is a good opportunity to revisit some of our design decisions.Screenshots
No response
Configuration
Additional context
No response
The text was updated successfully, but these errors were encountered: