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

remove `set x[1] x[2] a b` syntax #4236

Closed
krader1961 opened this Issue Jul 23, 2017 · 4 comments

Comments

Projects
None yet
3 participants
@krader1961
Copy link
Contributor

krader1961 commented Jul 23, 2017

This issue is to discuss removing a feature of the set command. Why? Because implementing support for tied variables (issue #4082) involves touching builtin_set(). Which is currently really hard to understand. So I wanted to refactor the src/builtin_set.cpp code into smaller components and eliminate all, or at least most, of the current lint warnings before doing the work for tied vars.

This caused me to notice a feature of the set command I was not aware of. From the man page:

If the variable name is one or more array elements, such as PATH[1 3 7], only those array elements specified will be changed. When array indices are specified to set, multiple arguments may be used to specify additional indexes, e.g. set PATH[1] PATH[4] /bin /sbin.

We don't have a single unit test for that second, alternative, syntax. Nor is there a single use in the fish project. Nor is there a single use in the Fisherman or Oh-My-Fish scripts.

This feature greatly complicates the code. It is also ambiguous and likely to be the source of errors if people actually used it. It also doesn't provide any functionality we don't already support via set PATH[1 4] /bin /sbin.

Run these two commands and examine the result: set x a b c d then set x[1 2] x[4] a. Now do set x[1 2] x[4] 1 2 3 and examine the result.

All in all I think fish 3.0 is the perfect time to remove support for this syntax.

@krader1961 krader1961 added this to the fish-3.0 milestone Jul 23, 2017

@krader1961 krader1961 self-assigned this Jul 23, 2017

@faho

This comment has been minimized.

Copy link
Member

faho commented Jul 24, 2017

Wow... I've never heard of that. It's inconsistent with every other use of variables, with set itself (I've always expected that the first token that doesn't start with a "-" is the variable name), it's clumsy (the variable name needs to be given twice, and two different variable names don't work) removes the theoretical possibility of setting the first element of a variable to something that looks like a variable with an index (i.e. setting foo[1] to something like "bar[x]")... And, worst of all, like you said, there's already a better alternative.

Yes, please ditch it!

@krader1961

This comment has been minimized.

Copy link
Contributor

krader1961 commented Jul 24, 2017

Wow... I've never heard of that.

I too hadn't noticed this "feature" in the docs. I only noticed because the code I was trying to refactor didn't make any sense until I read the set man page again. Then I realized the code could be radically simplified if we removed this, seemingly never used, feature.

krader1961 added a commit to krader1961/fish-shell that referenced this issue Jul 24, 2017

first step in refactoring the `set` implementation
The *src/builtin_set.cpp* code needs a major refactoring. This is the
first baby step in doing so.

Partial fix for fish-shell#4236

krader1961 added a commit to krader1961/fish-shell that referenced this issue Jul 24, 2017

refactor `set` builtin
This completes the refactoring of the `set` builtin. It also removes a
seemingly never used feature of the `set` command. It also eliminates all
the lint warnings about this module.

Fixes fish-shell#4236

krader1961 added a commit that referenced this issue Jul 24, 2017

refactor `set` builtin
This completes the refactoring of the `set` builtin. It also removes a
seemingly never used feature of the `set` command. It also eliminates all
the lint warnings about this module.

Fixes #4236

@krader1961 krader1961 removed the RFC label Jul 24, 2017

@krader1961

This comment has been minimized.

Copy link
Contributor

krader1961 commented Jul 24, 2017

Done. Fish 3.0 no longer supports the alternative syntax. The builtin_set() function and related code is also a lot simpler and all twelve lint warnings have been eliminated.

@krader1961 krader1961 closed this Jul 24, 2017

@cben

This comment has been minimized.

Copy link
Contributor

cben commented Aug 13, 2018

👍 to the simplification.
I have a guess why it was there:
This worked in fish 2.7.1:

$ set indexes 1 2 4
$ set array a b c d e f g h
$ echo $array[$indexes]
a b d
$ echo set array[$indexes] A B D
set array[1] array[2] array[4] A B D
$ set array[$indexes] A B D
$ echo $array
A B c D e f g h

The way it works is ugly but usage is pleasantly symmetrical.

On master reading a var with multiple indexing still works of course, but setting errors:

$ echo $array[$indexes]
a b d
$ set array[$indexes] A B D
set: You provided 1 indexes but 5 values

However, this works 🎉:

$ set "array[$indexes]" A B D
$ set array["$indexes"] A B D  # equivalent
$ echo $array 
A B c D e f g h

It was quite unobvious to me "array[$indexes]" would even work, I only tried it on a wild hunch... But set array[1 2 4] is by far cleaner & safer than set array[1] array[2] .... And discoverability can to some degree be helped by docs (I haven't re-read them in a long time).

EDIT: I'll send a docs PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment