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

defaults: Revert "defaults: Revert to old comparison" #1922

Conversation

gaelicWizard
Copy link
Contributor

@gaelicWizard gaelicWizard commented Aug 22, 2021

The change in #1918 (reverting #1913 / #188) breaks case-insensitive domain completion. Example: defaults read com.apple.term<TAB> should complete to defaults read com.apple.Terminal, but it does not due to the change from =~ to ==. This was the reason for the initial PR.

@gaelicWizard gaelicWizard force-pushed the revert-1918-fix-defaults-completion branch from 9e869d5 to ad634e7 Compare August 22, 2021 20:10
@gaelicWizard gaelicWizard changed the title Revert "defaults: Revert to old comparison" defaults: Revert "defaults: Revert to old comparison" Aug 22, 2021
Copy link
Contributor

@davidpfarrell davidpfarrell left a comment

Choose a reason for hiding this comment

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

I don't know how the existing == check ever worked. I can't find any docs that show that the syntax is correct

=~ [${cmds// /|}] does not do what you think

$ cmds='read read-type write rename delete domains find help'
[[ "w" =~ [${cmds// /|}] ]] && echo WTF

cmds is the fixed list of acceptable commands, why would it need to be case-insensitive? Does defaults accept case-insensitive commands ?

There are 3 uses of this check in the file - Its not clear why, twice now, you've only modified one use?

A technique that could work:

[[ "read-type" =~ ^(${cmds// /|})$ ]] && echo NICE

Another known technique, possibly what the original was going for (requires extglob):

[[ "read-type" == @(${cmds// /|}) ]] && echo NICE

We introduced _bash-it-array-contains-element in #1877 to not rely on regexes for this type of check.
We control the file, so we could easily convert the cmds value to an array and check against it.

@NoahGorny This completion look OLD - Wondering if we should see about finding an updated version and possibly vendoring it?

@NoahGorny
Copy link
Member

I have searched far and wide for the original completion repo, but was unable to find it unfortunately..
This is actually pretty annoying that we have this burden of old completions, written a long time ago, that we need to maintain.

Seems to me like this whole file is really old and not really working. I will try to take another look this week, but I am really against spending too much time on this old completion file.

In any case- I am happy that you are invested on this @gaelicWizard, and we need to see how we correctly do it, as this new way also introduces problems..

@gaelicWizard
Copy link
Contributor Author

Sorry for disappearing for a while! I don't actually know much about Bash's completion system, and this wasn't my patch.

Please note that Bash's [[ syntax treats unquoted second comparators as patterns, so changing =~ to == doesn't change anything. I wouldn't have submitted this PR to undo the change if I understood that two weeks ago!

In any case, I'm not the best person to work on completions stuff. Feel free to close this or fix it whatever you think is best.

@gaelicWizard
Copy link
Contributor Author

I'm closing this PR as I accidentalied #1928... 😆

@gaelicWizard gaelicWizard deleted the revert-1918-fix-defaults-completion branch September 9, 2021 04:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants