Skip to content

Add feature flag to turn off %self#10262

Merged
faho merged 2 commits into
fish-shell:masterfrom
faho:remove-percent-self
Feb 6, 2024
Merged

Add feature flag to turn off %self#10262
faho merged 2 commits into
fish-shell:masterfrom
faho:remove-percent-self

Conversation

@faho
Copy link
Copy Markdown
Member

@faho faho commented Jan 25, 2024

This is the last remnant of the old percent expansion.

It has the downsides of it, in that it is annoying to combine with anything:

echo %self/foo

prints "%self/foo", not fish's pid.

We have introduced $fish_pid in 3.0, which is much easier to use - just like a variable, because it is one.

If you need backwards-compatibility for < 3.0, you can use the following shim:

set -q fish_pid
or set -g fish_pid %self

So we introduce a feature-flag called "remove-percent-self" to turn it off.

"%self" will simply not be special, e.g. echo %self will print "%self".

Current uses of %self:

Here's a few notable uses of %self I can still find in the wild:

There are in total 386 matches on Github I can find. These are all easily, almost mechanically replaced with $fish_pid, with optional backwards-compatibility by setting the variable yourself if it doesn't exist (see above).

It is worth pointing out that $fish_pid was introduced in fish 3.0, which is 45% of commits up to 3.7.0 behind. It was released 5 years ago.

This is the last remnant of the old percent expansion.

It has the downsides of it, in that it is annoying to combine with
anything:

```fish
echo %self/foo
```

prints "%self/foo", not fish's pid.

We have introduced $fish_pid in 3.0, which is much easier to use -
just like a variable, because it is one.

If you need backwards-compatibility for < 3.0, you can use the
following shim:

```fish
set -q fish_pid
or set -g fish_pid %self
```

So we introduce a feature-flag called "remove-percent-self" to turn it
off.

"%self" will simply not be special, e.g. `echo %self` will print
"%self".
@faho faho added this to the fish next-3.x milestone Jan 25, 2024
@epidemian
Copy link
Copy Markdown
Contributor

I didn't know fish had this expansion of %self. If the goal is to eventually remove it, couldn't it first be deprecated —maybe printing a message like Warning: %self is deprecated and will be removed in a future version of fish. Please use $fish_pid instead— so that users can catch the remaining uses of this and report/fix them?

I understand this PR is not yet disabling this, just adding an opt-out feature flag for it. But i was wondering if a deprecation warning message could be useful too.

@faho
Copy link
Copy Markdown
Member Author

faho commented Jan 25, 2024

If the goal is to eventually remove it, couldn't it first be deprecated

This is the deprecation.

We're telling you that we're planning to turn it off, and individual users can already try it out.

And that is also how users catch it. In the meantime, the status quo is untouched, everything still works as before.

The next phase, some time in the future, is to disable %self by default, but allow you to turn it back on.

After that, it will eventually be permanently removed.

Warnings just make it more annoying, especially in this case where finding uses is trival - you can actually just run grep '%self'.

In particular, in this case, either we turn on the warnings by default and then every end user sees them when they use an affected plugin, and they're not the ones to fix it. Or we have them opt-in and then we're no better off than allowing you to disable it yourself. Especially when finding uses of %self is that easy.

@epidemian
Copy link
Copy Markdown
Contributor

Ok, that's fair, thanks for your thorough reply.

I was thinking more from the perspective of:

  1. I'm a regular fish user. I add some functions that use %self to my fish config.
  2. Years later, my package manager has a new version of fish. I let it update, as i always do. I don't check the changelog, and even if i did, by now i have completely forgotten about the detail of how my functions are implemented. I still use them every now and then.

If this version starts warning about uses of %self, i have a chance of catching them and i also get to learn how to fix them, while things still work. But if it doesn't warn, then later, when the version that disables this old behavior by default is released, my functions will suddenly stop working, without any indication of why.

I don't really know if this is too far fetched. It probably is. But it's the kind of use-case that came immediately to mind 😅

Comment thread tests/checks/features-percent-self1.fish Outdated
Comment thread doc_src/language.rst
qmark-noglob on 3.0 ? no longer globs
regex-easyesc on 3.1 string replace -r needs fewer \\'s
ampersand-nobg-in-token on 3.4 & only backgrounds if followed by a separating character
remove-percent-self off 3.8 %self is no longer expanded (use $fish_pid)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

sounds fine to me. Actually turning it on is a separate decision, possibly not for many years

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

It will be a while, yes, but I would imagine shorter than qmark-noglob.

I expect fewer issues than with stderr-nocaret - that one was bloody everywhere, and it was harder to find than this.

%self was never a huge thing, the main reason why there are still uses is that oh-my-fish is dead more than it being important.

@faho faho merged commit 8d71eef into fish-shell:master Feb 6, 2024
@faho faho deleted the remove-percent-self branch February 6, 2024 21:13
@github-actions github-actions Bot locked as resolved and limited conversation to collaborators Feb 10, 2025
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.

3 participants