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.

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

Support FOO=bar syntax for passing variables to individual commands #6287

Merged
merged 1 commit into from Nov 25, 2019

Conversation

krobelus
Copy link
Member

@krobelus krobelus commented Nov 4, 2019

This adds initial support for statements with prefixed variable assignments.
Statments like this are supported:

a=1 b=$a echo $b # outputs 1

Just like in other shells, the left-hand side of each assignment must
be a valid variable identifier (no quoting/escaping). Array indexing
(PATH[1]=/bin ls $PATH) is not yet supported, but can be added fairly
easily.

The right hand side may be any valid string token, like a command
substitution, or a brace expansion.

Since a=* foo is equivalent to begin set -lx a *; foo; end,
the assignment, like set, uses nullglob behavior, e.g. below command
can safely be used to check if a directory is empty.

x=/nothing/{,.}* test (count $x) -eq 0

Generic file completion is done after the equal sign, so for example
pressing tab after something like HOME=/ completes files in the
root directory
Subcommand completion works, so something like
GIT_DIR=repo.git and command git correctly calls git completions
(but the git completion does not use the variable as of now).

The variable assignment is highlighted like an argument.

Closes #6048


TODOs:

  • Changes to fish usage are reflected in user documentation/manpages.
  • Tests have been added for regressions fixed
  • User-visible changes noted in CHANGELOG.md

Follow-ups:

  • Figure out what to do for bare a=b
  • Figure out if export should work the same way

@faho
Copy link
Member

faho commented Nov 7, 2019

The FAQ contains a question about this that would have to be removed.

How do I set an environment variable for just one command?

@zanchey
Copy link
Member

zanchey commented Nov 8, 2019

Rather than removing the entry, what about changing it to "Since fish 3.x, the SOME_VAR=1 command syntax can be used, like other shells."?

@krobelus
Copy link
Member Author

krobelus commented Nov 9, 2019

I added this to the first FAQ entry, that should make sense but feel free to change it.

src/complete.cpp Outdated Show resolved Hide resolved
sphinx_doc_src/cmds/set.rst Outdated Show resolved Hide resolved
@faho
Copy link
Member

faho commented Nov 10, 2019

A question is how we should handle bare "VAR=VALUE" without any command.

Should we actually support foo=bar in addition to set? Or should we specifically block that?

@faho faho added this to the fish 3.2.0 milestone Nov 10, 2019
@zx8
Copy link

zx8 commented Nov 12, 2019

Given the fact that fish already "supports" export FOO=bar syntax (albeit via a horrible hack) to help newcomers transition to fish, my opinion is that we should support bare var=val syntax, purely for convenience.

I often find myself following guides, studying online courses, or simply being sent shell snippets from colleagues that assume bare var=val syntax exists, and it's cumbersome having to manually change them all to set var val, when the script would otherwise have worked verbatim.

@krobelus
Copy link
Member Author

Adding support for var=val seems reasonable to make fish more copy-paste friendly. Its use in scripts should probably be discouraged.

We also need to settle on what to do when the value expands to multiple elements.
Which probably happens in a quite small minority of all uses but the current behavior in this branch is inconsistent:

$ export a=(printf 1\n2\n); printf %s, $a
2,
$ a=(printf 1\n2\n) printf %s, $a
1,2,

@zanchey
Copy link
Member

zanchey commented Nov 13, 2019

My problem with bare FOO=bar being equivalent to set FOO bar is that it is internally inconsistent - bare it produces a persistent (non-exported) shell variable, but combined with a command it is exported and limited only to that command.

Copy link
Member

@ridiculousfish ridiculousfish left a comment

Choose a reason for hiding this comment

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

Overall this is really fantastic - fits in nicely and well designed. This is thrilling!

I haven't fully finished reviewing the execution part yet; I expect it to change a bit if we take the grammar simplification which I suggest.

src/parse_grammar.h Outdated Show resolved Hide resolved
src/parse_tree.cpp Outdated Show resolved Hide resolved
src/parse_execution.cpp Outdated Show resolved Hide resolved
@ridiculousfish
Copy link
Member

Looks great! Merge as you like.

I don't have any strong feelings about the behavior of bare FOO=var but we should at least improve the error message, which is currently:

foo=bar
fish: Expected a command, but instead found end of the statement
foo=bar
        ^

This adds initial support for statements with prefixed variable assignments.
Statments like this are supported:

a=1 b=$a echo $b        # outputs 1

Just like in other shells, the left-hand side of each assignment must
be a valid variable identifier (no quoting/escaping).  Array indexing
(PATH[1]=/bin ls $PATH) is *not* yet supported, but can be added fairly
easily.

The right hand side may be any valid string token, like a command
substitution, or a brace expansion.

Since `a=* foo` is equivalent to `begin set -lx a *; foo; end`,
the assignment, like `set`, uses nullglob behavior, e.g. below command
can safely be used to check if a directory is empty.

x=/nothing/{,.}* test (count $x) -eq 0

Generic file completion is done after the equal sign, so for example
pressing tab after something like `HOME=/` completes files in the
root directory
Subcommand completion works, so something like
`GIT_DIR=repo.git and command git ` correctly calls git completions
(but the git completion does not use the variable as of now).

The variable assignment is highlighted like an argument.

Closes fish-shell#6048
@krobelus krobelus merged commit 7d5b44e into fish-shell:master Nov 25, 2019
@krobelus krobelus modified the milestones: fish 3.2.0, fish 3.1.0 Nov 25, 2019
krobelus added a commit to krobelus/fish-shell that referenced this pull request Nov 25, 2019
Since fish-shell#6287, bare variable assignments do not parse, which broke
the "Unsupported use of '='" error message.

This commit catches parse errors that occur on bare variable assignments.
When a statement node fails to parse, then we check if there is at least one
prefixing variable assignment. If so, we emit the old error message.

See also fish-shell#6347
krobelus added a commit to krobelus/fish-shell that referenced this pull request Nov 26, 2019
Since fish-shell#6287, bare variable assignments do not parse, which broke
the "Unsupported use of '='" error message.

This commit catches parse errors that occur on bare variable assignments.
When a statement node fails to parse, then we check if there is at least one
prefixing variable assignment. If so, we emit the old error message.

See also fish-shell#6347
krobelus added a commit that referenced this pull request Nov 26, 2019
Since #6287, bare variable assignments do not parse, which broke
the "Unsupported use of '='" error message.

This commit catches parse errors that occur on bare variable assignments.
When a statement node fails to parse, then we check if there is at least one
prefixing variable assignment. If so, we emit the old error message.

See also #6347
@lilyball
Copy link
Contributor

I'm not in a position to build fish at the moment, but one question that I don't think was raised in the original ticket: What's the behavior of

a=foo b=$a echo $b

Is b set to foo or is it set to the previous value of a from before this statement?

@krobelus
Copy link
Member Author

a=foo b=$a echo $b prints foo, not the previous value of $a

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Apr 16, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Consider supporting FOO=bar cmd syntax
6 participants