funced: pretend copied functions are defined interactively #11691
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
krobelus
reviewed
Aug 6, 2025
CHANGELOG.rst
Outdated
| @@ -51,6 +51,7 @@ Interactive improvements | |||
| - Instead of flashing all the text to the left of the cursor, fish now flashes the matched token during history token search, the completed token during completion (:issue:`11050`), the autosuggestion when deleting it, and the full command line in all other cases. | |||
| - Pasted commands are now stripped of any ``$`` prefix. | |||
| - The :kbd:`alt-s` binding will now also use ``run0`` if available. | |||
| - ``funced`` will now edit copied functions directly, instead of the file where it was copied. (:issue:`11614`) | |||
There was a problem hiding this comment.
This is technically correct but maybe it's clearer to say
maybe « ... where "functions --copy" was invoked » ?
I'm fine eitehr way.
There was a problem hiding this comment.
I have changed it to instead of the file where ``function --copy`` was invoked.
share/functions/funced.fish
Outdated
| @@ -71,6 +71,11 @@ function funced --description 'Edit function definition' | |||
|
|
|||
| if not functions -q -- $funcname | |||
| echo $init >$tmpname | |||
| else if string match --invert --quiet --regex '^(?:n/a|not-autoloaded|autoloaded)$' -- (functions --details --verbose -- $funcname)[2] | |||
There was a problem hiding this comment.
Probably not string match would be a little bit more idiomatic than string match --invert.
It's a shame that we don't have an easier / more robust way of figuring out that a function was copied from somewhere.
There was a problem hiding this comment.
The line later had that --invert so I had kept it. changed it to not.
| # Pretend this copied function does not have a definition file. Editing the file which | ||
| # originally defined it would not update this copy, and the file which copied it | ||
| # would not have a function body to edit. (issue #11614) | ||
| functions -- $funcname >$tmpname |
There was a problem hiding this comment.
Ok, after testing this, I think I get it. LGTM, thanks for fixing this.
Your PR description makes sense - maybe put it into the commit message?
Here's a quick test, run with
cargo b && tests/test_driver.py target/debug tests/checks/funced.fish
(we could definitely add more tests)
diff --git a/tests/checks/funced.fish b/tests/checks/funced.fish
new file mode 100644
index 0000000000..a285635334
--- /dev/null
+++ b/tests/checks/funced.fish
@@ -0,0 +1,22 @@
+#RUN: %fish %s
+
+function my-src
+ echo hello
+end
+
+echo >my-copy-function.fish "functions --copy my-src my-dst"
+source my-copy-function.fish
+
+functions --details --verbose my-dst
+# CHECK: my-copy-function.fish
+# CHECK: {{.*}}tests/checks/funced.fish
+# CHECK: 3
+# CHECK: scope-shadowing
+
+EDITOR=cat funced my-dst
+# CHECK: # Defined in {{.*}}/tests/checks/funced.fish @ line 3, copied in my-copy-function.fish @ line 2
+# CHECK: function my-dst
+# CHECK: echo hello
+# CHECK: end
+# CHECK: Editor exited but the function was not modified
+# CHECK: If the editor is still running, check if it waits for completion, maybe a '--wait' option?There was a problem hiding this comment.
Thank you. I have added the test.
I have also rebased the commit and updated the commit message.
krobelus
approved these changes
Aug 6, 2025
5caaf86 to
1b1df8c
Compare
After fish-shell#9542, the format for `functions -Dv` was changed for copied functions. ```diff -stdin -n/a +[path to copy location] +[path to original definition] [and a few more lines] ``` Some components were (and perhaps are) still expecting the old format, however. After a search, it looks like `funced` and `fish_config` are the only two functions using `functions -D` and `functions -Dv` in this repo (none are using `type -p`). As noted in issue fish-shell#11614, `funced` currently edits the file which copies the given copied function. Another option was to make `funced` edit the file which originally defined the function. Since the copied function would not have been updated either way, I modified `funced` so it would pretend that the copied function was defined interactively, like it was before. I did not modify `fish_config`, since it was only used for preset prompts in the web config, none of which used `functions --copy`. (Moreover, I believe it would have behaved correctly, since the preset would not have had to define the function, only copy it.) Fixes issue fish-shell#11614
1b1df8c to
45bb8f5
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
After #9542, the format for
functions -Dvwas changed for copied functions.Some components were (and perhaps are) still expecting the old format, however. After a search, it looks like
funcedandfish_configare the only two functions usingfunctions -Dandfunctions -Dvin this repo (none are usingtype -p).As noted in issue #11614,
funcedcurrently edits the file which copies the given copied function. Another option was to makefuncededit the file which originally defined the function. Since the copied function would not have been updated either way, I modifiedfuncedso it would pretend that the copied function was defined interactively, like it was before.(I did not find any preexisting test file for
funced, so I did not write regressions test. Please tell me if I should.)I did not modify
fish_config, since it was only used for preset prompts in the web config, none of which usedfunctions --copy. (Moreover, I believe it would have behaved correctly, since the preset would not have had to define the function, only copy it. (This grammar is making me dizzy.))Fixes issue #11614
TODOs: