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

Make ":" and "." proper builtins #6854

Closed
wants to merge 6 commits into from
Closed

Conversation

faho
Copy link
Member

@faho faho commented Apr 4, 2020

Description

We've tried to remove "." before, to disastrous results, so these aren't going anywhere. Since it's simpler to implement them in C++ than script, and since that makes fish just a little more functional without share/config.fish, I'd like to add them to C++ and make them real builtins.

TODOs:

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

faho added 3 commits April 4, 2020 13:17
Should be a bit easier to read.

[ci skip]
It's *less code* to define this as a builtin, and it's not going
anywhere. Plus it makes fish just a little more usable without share/config.fish.
Yeah, it's not going anywhere. This is one line in builtin.cpp vs 9
lines of script, most of which used to print an error that is never triggered.
@faho faho added the cleanup label Apr 4, 2020
@faho faho added this to the fish 3.2.0 milestone Apr 4, 2020
This used to match a line

   complete -c . --wraps source

but since `.` is no longer a function we no longer set up wrapping.
Since I don't think we do anything special for `source`s completions,
it's okay to remove.
@krobelus
Copy link
Member

krobelus commented Apr 4, 2020

Sounds good.
. should still have file completions, and I prefer : to have them as well, but I'm happy to be overruled here.

diff --git a/share/functions/__fish_config_interactive.fish b/share/functions/__fish_config_interactive.fish
index faef0ef05..1e62b96b2 100644
--- a/share/functions/__fish_config_interactive.fish
+++ b/share/functions/__fish_config_interactive.fish
@@ -168,7 +168,7 @@ function __fish_config_interactive -d "Initializations that should be performed
     #
     # Only a few builtins take filenames; initialize the rest with no file completions
     #
-    complete -c(builtin -n | string match -rv '(source|cd|exec|realpath|set|\\[|test|for)') --no-files
+    complete -c(builtin -n | string match -rv '^(.|:|source|cd|exec|realpath|set|\\[|test|for)$') --no-files
 
     # Reload key bindings when binding variable change
     function __fish_reload_key_bindings -d "Reload key bindings when binding variable change" --on-variable fish_key_bindings

faho added 2 commits April 4, 2020 20:31
For `.` it's *correct* and for `:` it literally accepts everything
For `true`, this makes uses like the

    : some description of the job &

we used to have impossible, also it's just *wrong* that true can
return something that isn't true.

For false it's not super important but it should generally be
symmetrical with true.
@faho
Copy link
Member Author

faho commented Apr 4, 2020

Okay, there is one more issue here: Our true actually errors (with a non-true status!) if it receives arguments, so this would break use like the : job description bit we used to mark what the python in the background was doing.

That seems quite wrong to begin with - true should just return true.

@@ -168,7 +168,7 @@ function __fish_config_interactive -d "Initializations that should be performed
#
# Only a few builtins take filenames; initialize the rest with no file completions
#
complete -c(builtin -n | string match -rv '(source|cd|exec|realpath|set|\\[|test|for)') --no-files
complete -c(builtin -n | string match -rv '(.|:|source|cd|exec|realpath|set|\\[|test|for)') --no-files
Copy link
Member

Choose a reason for hiding this comment

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

I think the . has to be escaped here.

@ridiculousfish
Copy link
Member

ridiculousfish commented Apr 5, 2020

As-is this outputs text like complete -c '[' on launch; escaping the . fixes it. Changes LGTM otherwise.

@faho
Copy link
Member Author

faho commented Apr 6, 2020

Merged as be0de5e..4eccc0f, with the \. fix.

Note that also changes true and false to accept any number of arguments - as they should.

@faho faho closed this Apr 6, 2020
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jul 5, 2020
@faho faho deleted the init-cleanup branch September 23, 2021 09:41
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants