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

Rename $_ #813

Closed
dag opened this Issue May 23, 2013 · 20 comments

Comments

Projects
None yet
8 participants
@dag
Copy link
Contributor

dag commented May 23, 2013

The name is inconsistent with the rest of fish and as far as I know not even consistent with other shells. Not even that consistent with Perl's topic variable!

@xfix

This comment has been minimized.

Copy link
Member

xfix commented May 23, 2013

Indeed. $_ doesn't deserve to have such short name, considering it only contains currently running program. Especially considering it's only useful inside fish_title.

If anything deserves $_, it's only $argv ($_[1] would look like Perl). But $argv isn't that bad, so I think that $_ should be simply unused. Symbol variables don't have to be used, really, fish isn't POSIX (and besides, $_ isn't even POSIX).

Also, considering other shells actually use $_ for their purposes, I don't think fish should use it, in order to avoid confusion.

@dag

This comment has been minimized.

Copy link
Contributor

dag commented May 23, 2013

Ah, I thought it was also like bash's $0 when running a script file, but seems not.

@zanchey

This comment has been minimized.

Copy link
Member

zanchey commented Dec 11, 2014

There doesn't actually seem to be an equivalent of $0, which is annoying. Perhaps status could be extended.

@krader1961

This comment has been minimized.

Copy link
Contributor

krader1961 commented Dec 13, 2016

It is unfortunate this var was named $_ given that in ksh and bash it refers to the last argument of the previous command rather than the command as in fish. Nonetheless, it is far too late to change this as doing so breaks backward compatibility for very little benefit.

It might be useful to introduce an alias. Which would allow us in the future, when we create the fish 3.0 release, to remove $_ or change its definition. Does anyone have a recommendation for a more explicit var name?

@mqudsi

This comment has been minimized.

Copy link
Contributor

mqudsi commented Mar 11, 2018

I've committed this in d367d57 with $_ renamed to $current_cmd.

If there are any objections to the name or better suggestions, please chime in!

@faho

This comment has been minimized.

Copy link
Member

faho commented Mar 13, 2018

If there are any objections to the name or better suggestions, please chime in!

I don't think removing it right now is worth it. I'd rather have both names for a while, and deprecate the old one.

Also, I don't really like how many variables with generic names we're now using - $current_cmd is actually a name someone might have used, as is $hostname.

@zanchey

This comment has been minimized.

Copy link
Member

zanchey commented Mar 13, 2018

Please back this out. I agree with @faho entirely.

@mqudsi

This comment has been minimized.

Copy link
Contributor

mqudsi commented Mar 13, 2018

Sure thing. If you'll read my commit logs, you'll see similar remarks to your own reservations. I feel $_ should probably become status --something, and I also have huge reservations about using such common names (see commitlog for my $hostname implementation as well). There was already a very heated and acrimonious debate about $version and some commits one way and then the other, if I'm not mistaken, and I didn't want to get involved in that so I said that I'm just sticking to what we're currently using (no $fish_ prefix) and we can change them or keep them as the team decides.

@faho

This comment has been minimized.

Copy link
Member

faho commented Mar 13, 2018

debate about $version

Yes, there was. The main problem I could see is that $version is something you use to check which version of fish you are dealing with, so it's important that it stays consistent. And unfortunately it was named "$version" from the beginning.

We have a few more variables with generic names - $status, $argv - but those are front and center, something you use constantly. Others, like $PATH or $LANG, are standardized across shells. So there's a reasonable expectation you might know them.

This stuff - $pid, $current_command, $hostname - has names that might already be in use in user's scripts, or that users might want to use. And they're used much less often, so users might not know or might have forgotten about them.


So, what would I use?

For $pid, I'd use $fish_pid - because that makes it clear that it's actually the pid of fish.

For $last_pid, I'd tell people to use jobs, and perhaps improve that.

For $hostname, I'm inclined to go with the pseudo-standard $HOSTNAME or $HOST.

For $_, yeah, I'm okay with status current-command. We already have the filename and line-number in there, so the command fits.

But the important thing is that we keep the old versions for a while (as in the next 2-5 years). There really is no reason to rush these cosmetics.

(I feel less strongly about keeping "%" expansion, though, because it's barely used and sucks. If we kept "%self" as a special case that would solve 99% of uses)

@faho faho reopened this Mar 13, 2018

@mqudsi

This comment has been minimized.

Copy link
Contributor

mqudsi commented Mar 13, 2018

I am all for $fish_pid, that was my first preference by a mile. $last_pid is actually very misleading since it doesn't actually return the last pid but rather the pid of the last backgrounded job, so exclusively tying it to jobs is probably the way to go.

Now that I think about it, there's no need for $hostname to be "exposed" at all. We can actually use a "private" $__hostname since we only call it in the prompt_hostname function, which people can feel free to call instead?

I'm open to one other suggestion here: we can call it $HOSTNAME, but only if a) we export it (I agree with whoever it was that said all caps should be reserved for exported variables), and b) we respect any existing environment variable value with that name, i.e. we only perform a hostname lookup if the HOSTNAME environment variable is not set. Which might just be the most sensible option, come to think of it.

@mqudsi

This comment has been minimized.

Copy link
Contributor

mqudsi commented Mar 13, 2018

Oh, and with regards to %: that was so poorly spec'd and functionally broken, that using anything other than %self or %last was like playing Russian roulette. The process expansion by name, even if it were implemented correctly in fish, is such a boneheaded way to blindly guess which pid the user actually wants, and while %1 may have worked for retrieving the pid of a job, what constitutes a "job" is definitely very fluid.

I mentioned elsewhere my original idea of taking advantage of the fact that % is no longer a special variable to create a %self function that would emit a deprecation notice to stderr and then return the value of $fish_pid to stdout, but then realized that obviously wouldn't work since functions must be called in a subshell while %self would have expanded anywhere (including at the head of a command, where fish normally prevents expansions from being performed). At any rate, searching for usages of %last or %self and replacing them with their new replacements is definitely straightforward.

@zanchey

This comment has been minimized.

Copy link
Member

zanchey commented Mar 15, 2018

Can we keep %last and %self? Do they do any harm?

@mqudsi

This comment has been minimized.

Copy link
Contributor

mqudsi commented Mar 15, 2018

They reserve the % character. I guess a hack could be written to have it reserved only in combination with those two tokens.

@krader1961

This comment has been minimized.

Copy link
Contributor

krader1961 commented Mar 23, 2018

FWIW, You need to revert commit d367d57 ASAP. See the discussion in Gitter started by @nafg. That commit breaks updating the terminal title since, among other uses, the default title function uses the old var:

#define DEFAULT_TITLE L"echo $_ \" \"; __fish_pwd"

You cannot blindly replace a public var in this fashion without a lengthy transition period.

@krader1961

This comment has been minimized.

Copy link
Contributor

krader1961 commented Mar 23, 2018

My previous comment also applies to several other recent var renames of this sort. Just because those renames are happening in a major release does not mean you can unilaterally invalidate the old var name.

@nafg

This comment has been minimized.

Copy link

nafg commented Mar 23, 2018

@mqudsi

This comment has been minimized.

Copy link
Contributor

mqudsi commented Mar 24, 2018

Mea culpa. I searched .*\.fish files for \$_\b, so I missed out on that DEFAULT_TITLE declaration.

Again, though, the intention was always to provide a shim for compatibility before 3.0, but was soliciting feedback on the best approach (ref commit msgs). It would have been better to open a PR instead of commit directly in that case, so I take full responsibility.

@jorgebucaran

This comment has been minimized.

Copy link

jorgebucaran commented Sep 24, 2018

I suppose $_ will still be available in fish 3.0.0 though? A deprecation warning would be nice, but shipping 3.0.0 already without $_ would be too drastic.

I also agree with @krader1961 in that it is too late to change this as doing so breaks backward compatibility for virtually no benefit. I know $_ isn't a great name, but at least it doesn't use up a name that could make a valid variable name otherwise.

@faho

This comment has been minimized.

Copy link
Member

faho commented Sep 24, 2018

I suppose $_ will still be available in fish 3.0.0 though?

@jorgebucaran: Yes, it's still there.

@jorgebucaran

This comment has been minimized.

Copy link

jorgebucaran commented Sep 24, 2018

@faho Good to know. Thanks! 🎉

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