Refactor completion for `defaults` command #1928

gaelicWizard · 2021-09-07T03:46:55Z

Description

I accidentally rewrote the completion functions for the Mac OS X defaults command. This version is case-insensitive for all completions, uses $IFS to allow for spaces in domain and value names, and works for all my ad hoc testing.

Motivation and Context

This was an offshoot from another issue and PR and I ended up down the rabbit hole. But now I know way more about Bash's programmable completion!

The goal was to allow case-insensitive completion of domain names, for which an existing PR was pending. That PR had some weirds in it and ended up not actually achieving its goal, so I accidentally fixed it.

How Has This Been Tested?

Tested locally, and all tests pass.

Types of changes

Bug fix (non-breaking change which fixes an issue)

Checklist:

My code follows the code style of this project.
If my change requires a change to the documentation, I have updated the documentation accordingly.
I have read the CONTRIBUTING document.
If I have added a new file, I also added it to clean_files.txt and formatted it using lint_clean_files.sh.
I have added tests to cover my changes, and all the new and existing tests pass.

gaelicWizard · 2021-09-07T03:53:00Z

I've just noticed that using -currentHost/-host arguments breaks the completion entirely...

davidpfarrell

This looks like a legit effort to re-work the completion, and @gaelicWizard seems to be pretty invested it. So I'm guessing they use it regularly?

However: Regarding the use of =~ ${cmds// /|}:

With cmds being array, this simply does not work. ${cmds} simply resolves to read (i.e the first element in the array) - Its the default behavior in bash when referencing an array like a scalar.

I'm happy with the value being an array, but I suggest to use the _bash-it-array-contains-element method that we added for this purpose.

Additionally, if you decide to revert the usage of an array:

if its going to use regex (=~) to match, then per my previous comment you must anchor your values:
anchor with ^( and )$

=~ ^(${cmds// /|})$

This ensures that only fully matching words will match.

Additionally as mentioned, this works:

== @(${cmds// /|})

This is the technique I would probably go for if I didn't already have _bash-it-array-contains-element

Lastly:

Per discussion in #1924 , if jmah does decide to re-take ownership of the completion, I'm inclined to let them have it (it its original form), vendor it back into bash-it, and submit bug fixes upstream.

gaelicWizard · 2021-09-09T19:05:20Z

Per discussion in #1924 , if jmah does decide to re-take ownership of the completion, I'm inclined to let them have it (it its original form), vendor it back into bash-it, and submit bug fixes upstream.

Definitely.

gaelicWizard · 2021-09-09T19:06:37Z

With cmds being array, this simply does not work. ${cmds} simply resolves to read (i.e the first element in the array) - Its the default behavior in bash when referencing an array like a scalar.

I converted it to an array after that and...oops. I was fighting with a different part of it and ended up making lots of arrays instead of wordplitting. Back to the drawing board!

gaelicWizard · 2021-09-09T19:44:54Z

This ensures that only fully matching words will match.

We specifically want to match partial words; this is a completion handler! 😜

I suggest to use the _bash-it-array-contains-element method that we added for this purpose.

I had not thought to use _bash-it-array-contains-element! I absolutely would have if I had thought of it, but now that @jmah might be taking this upstream I feel like I now should make sure not to use it...

I've just reverted the change to making it an array and tested everything myself again. I think I need to write some BATS testing for this. Like I said before, completions are a new area of scripting to me. so many edge cases!

Thanks for helping me with this @davidpfarrell!

== @(${cmds// /|})
This is the technique I would probably go for

I've never seen this syntax before; @( ) is a new one to me. I'm going to try a version using that, but shellcheck complains when using == with a pattern so I'm not sure how that will play here. [[ uses unquoted second operands as patterns even with ==, so it still works, but it reads like it's comparing for equality and =~ is more clear that it's matching a pattern.

pretty invested it. So I'm guessing they use it regularly?

This is literally how I learned Bash It existed 😆

gaelicWizard · 2021-09-09T19:57:41Z

I've never seen this syntax before; @( ) is a new one to me.

It's part of extglob! I never use that stuff since it's off by default and I don't like to depend on it. Does Bash It turn that on? Can I rely on it here?

gaelicWizard · 2021-09-09T20:19:52Z

I simply cannot make this work with any form of ==, unless I wrap in @( ) with extglob turned on. It only works with =~ otherwise (which is bothering me b/c the second operand is supposed to be a pattern without quoting..?).

What am I missing?

gaelicWizard · 2021-09-09T22:31:56Z

Ok, this branch is working as-is and ready for merge, using =~. I have alsö prepared another commit that I can push which requires extglob and uses ==, iff someone tells me that Bash It requires extglob and we can rely on it here. (If requiring extglob in Bash It is documented somewhere already, please let me know.)

The version using == is case-sensitive for the verb (from $cmds), which is incorrect but I doubt anyone will ever notice. (Yes, the defaults command accepts the verb case-insensitively itself.)

@davidpfarrell, can we require extglob?

gaelicWizard · 2021-09-12T01:20:35Z

Mwahahaha! It's all over now! I've learned BATS! I'm unstoppable!! Mwahahahahahah!! 🤣🤪

Alsö, I totally just checked in a failing test to my previously-passing PR so...good job me. Anyway, should have that one fixed shortly.

NoahGorny · 2021-09-16T18:33:19Z

I don't know how to make @buhl's git-vendor fork work (it puts everything in repo root), and the dangling commit from upstream makes my brain worms riot. What do, @NoahGorny? 🥺

Hmm, did you already create a repo for the defaults completion? I'll try to vendor it myself 😄

NoahGorny · 2021-09-16T19:54:00Z

I dont see any repo that you opened @gaelicWizard, so let me know when you open one!

gaelicWizard · 2021-09-17T18:26:49Z

@NoahGorny, I've just put up https://github.com/gaelicWizard/bash-progcomp and tagged v2.0.0. I'll do a v2.0.1 and probably a v2.0.2 as I expand the BATS tests.

gaelicWizard · 2021-09-17T18:27:44Z

Either way, I figured out the benefits of @buhl's fork! I can amend commits! So, easy enough to just git mv and then --amend. I'll submit the vendor'd PR today

gaelicWizard · 2021-09-17T19:05:40Z

I did not close this. @NoahGorny?

NoahGorny · 2021-09-17T19:07:18Z

Are you sure?
To me it says that you did :O

gaelicWizard · 2021-09-17T22:35:35Z

Yeah, it says I did, but I wasn't able to re-open it then...but now I am...!! 🙃

gaelicWizard · 2021-09-17T22:36:08Z

Are you sure?
To me it says that you did :O

Sorry, I just realized that sounded like I was accusing you. I wasn't! I was asking you to help ♥

davidpfarrell · 2021-09-18T17:05:19Z

Since I've done a 2.0 of this, I'm going to vendor it out anyway.

Pulling things like this out of bash-it makes them more accessible to the community.

You are definitely the best person to own this particular completion - Makes me feel bad I don't use defaults more myself :)

Nice work !

NoahGorny · 2021-09-20T13:01:54Z

Either way, I figured out the benefits of @buhl's fork! I can amend commits! So, easy enough to just git mv and then --amend. I'll submit the vendor'd PR today

Hi @gaelicWizard, let me know if you need any help with the vendoring process- you rock 😃

gaelicWizard · 2021-10-12T19:12:31Z

@NoahGorny, I'm giving up on git vendor for now.

git vendor add bash-progcomp https://github.com/gaelicWizard/bash-progcomp v2.0.1

On a freshly cloned repo, this tells me

mv: illegal option -- t
usage: mv [-f | -i | -n] [-v] source target
       mv [-f | -i | -n] [-v] source ... directory

and then leaves me with a merge conflict and a dirty tree.

Halp?!

davidpfarrell · 2021-10-27T01:17:09Z

mv: illegal option -- t

Upon googling the error, It looks like the -t option is not available on MacOS (and possibly FreeBSD)

Maybe the internal git handler for the vendor action is invoking that?

You can install a gnu version of mv using with brew (see brew info coreutils).

I've done this on my mac.

The package actually installs the binaries with names like gmv by default, so you can pick and chose which ones you want to use.

There's also instructions on using them as the native names en-mass:

Caveats
Commands also provided by macOS and the commands dir, dircolors, vdir have been installed with the prefix "g".
If you need to use these commands with their normal names, you can add a "gnubin" directory to your PATH with:
  PATH="/usr/local/opt/coreutils/libexec/gnubin:$PATH"

Confirmed the gmv command comes with -t support:

$ man gmv

...
       -t, --target-directory=DIRECTORY
              move all SOURCE arguments into DIRECTORY
...

NOTE: The bash-it git aliases also have a gmv alias (for git mv) and will likely collide with the gnu gmv command.

What I did (just now):

I maintain my own ~/bin folder on the path for situations like this.

I did:

$ ln -s /usr/local/bin/gmv ~/bin/mv

$ mv --help
...
  -t, --target-directory=DIRECTORY  move all SOURCE arguments into DIRECTORY
...

Now just mv is configured to use the gnu version.

Hope that helps!

gaelicWizard · 2021-10-28T21:09:50Z

Thanks to @davidpfarrell, I just edited the git-vendor script to use a more normal syntax and now it works! It doesn't handle files starting with periods, but that was easy enough to hack around.

davidpfarrell

Best I can tell, this seems baked and ready to go.

/side note:
I'm not up to speed on our vendoring procedures, but is it our practice to vendor an entire repo? i.e instead of just the file(s) we need for execution + LICENSE ?

NoahGorny

Hi @gaelicWizard
Please rebase, and then we can merge this 😄

…omp@v2.0.1" git-vendor-name: bash-progcomp git-vendor-dir: vendor/github.com/gaelicWizard/bash-progcomp git-vendor-repository: https://github.com/gaelicWizard/bash-progcomp git-vendor-ref: v2.0.1

gaelicWizard · 2021-12-27T19:37:34Z

Rebased and ready to go 👍

gaelicWizard mentioned this pull request Sep 7, 2021

Discuss: Completion for Mac Defaults - The one where he hunts down Janathon Mah #1924

Closed

gaelicWizard force-pushed the defaults branch from 1f81b92 to 70ee0ac Compare September 7, 2021 03:54

This was referenced Sep 7, 2021

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

Closed

plugin/base: improvements #1930

Merged

gaelicWizard force-pushed the defaults branch from 70ee0ac to 8444bb7 Compare September 9, 2021 01:55

gaelicWizard marked this pull request as ready for review September 9, 2021 01:56

gaelicWizard changed the title ~~DRAFT: Refactor completion for defaults command~~ Refactor completion for defaults command Sep 9, 2021

gaelicWizard force-pushed the defaults branch from 8444bb7 to ca05f34 Compare September 9, 2021 03:52

davidpfarrell suggested changes Sep 9, 2021

View reviewed changes

gaelicWizard force-pushed the defaults branch from ca05f34 to 1812dd2 Compare September 9, 2021 19:29

gaelicWizard force-pushed the defaults branch from 1812dd2 to 35fe436 Compare September 9, 2021 20:51

gaelicWizard force-pushed the defaults branch 3 times, most recently from 83b3ca1 to 91a6c18 Compare September 11, 2021 21:42

gaelicWizard force-pushed the defaults branch 7 times, most recently from 9f9e3f3 to e7597b6 Compare September 12, 2021 03:04

gaelicWizard requested a review from davidpfarrell September 12, 2021 03:14

gaelicWizard closed this Sep 17, 2021

gaelicWizard force-pushed the defaults branch from 0502b06 to 9ea6002 Compare September 17, 2021 18:57

gaelicWizard reopened this Sep 17, 2021

gaelicWizard force-pushed the defaults branch from 238ec88 to 31cf790 Compare September 18, 2021 07:50

gaelicWizard force-pushed the defaults branch 2 times, most recently from e07d1d8 to cb80d65 Compare September 26, 2021 19:13

gaelicWizard requested a review from NoahGorny October 12, 2021 19:12

gaelicWizard force-pushed the defaults branch from 7d0d348 to a60abc0 Compare October 28, 2021 21:10

gaelicWizard marked this pull request as ready for review October 28, 2021 21:11

davidpfarrell approved these changes Oct 29, 2021

View reviewed changes

NoahGorny approved these changes Dec 10, 2021

View reviewed changes

gaelicWizard added 2 commits December 27, 2021 11:36

Remove defaults completion v1

fdb50ae

gaelicWizard force-pushed the defaults branch from a60abc0 to 6a41dbc Compare December 27, 2021 19:36

NoahGorny merged commit 475952d into Bash-it:master Jan 1, 2022

gaelicWizard deleted the defaults branch January 1, 2022 22:00

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor completion for `defaults` command #1928

Refactor completion for `defaults` command #1928

gaelicWizard commented Sep 7, 2021 •

edited

Loading

gaelicWizard commented Sep 7, 2021

davidpfarrell left a comment

gaelicWizard commented Sep 9, 2021

gaelicWizard commented Sep 9, 2021

gaelicWizard commented Sep 9, 2021

gaelicWizard commented Sep 9, 2021

gaelicWizard commented Sep 9, 2021 •

edited

Loading

gaelicWizard commented Sep 9, 2021

gaelicWizard commented Sep 12, 2021

NoahGorny commented Sep 16, 2021

NoahGorny commented Sep 16, 2021

gaelicWizard commented Sep 17, 2021

gaelicWizard commented Sep 17, 2021

gaelicWizard commented Sep 17, 2021

NoahGorny commented Sep 17, 2021

gaelicWizard commented Sep 17, 2021

gaelicWizard commented Sep 17, 2021

davidpfarrell commented Sep 18, 2021 •

edited

Loading

NoahGorny commented Sep 20, 2021

gaelicWizard commented Oct 12, 2021

davidpfarrell commented Oct 27, 2021

gaelicWizard commented Oct 28, 2021

davidpfarrell left a comment

NoahGorny left a comment

gaelicWizard commented Dec 27, 2021

Refactor completion for defaults command #1928

Refactor completion for defaults command #1928

Conversation

gaelicWizard commented Sep 7, 2021 • edited Loading

Description

Motivation and Context

How Has This Been Tested?

Types of changes

Checklist:

gaelicWizard commented Sep 7, 2021

davidpfarrell left a comment

Choose a reason for hiding this comment

gaelicWizard commented Sep 9, 2021

gaelicWizard commented Sep 9, 2021

gaelicWizard commented Sep 9, 2021

gaelicWizard commented Sep 9, 2021

gaelicWizard commented Sep 9, 2021 • edited Loading

gaelicWizard commented Sep 9, 2021

gaelicWizard commented Sep 12, 2021

NoahGorny commented Sep 16, 2021

NoahGorny commented Sep 16, 2021

gaelicWizard commented Sep 17, 2021

gaelicWizard commented Sep 17, 2021

gaelicWizard commented Sep 17, 2021

NoahGorny commented Sep 17, 2021

gaelicWizard commented Sep 17, 2021

gaelicWizard commented Sep 17, 2021

davidpfarrell commented Sep 18, 2021 • edited Loading

NoahGorny commented Sep 20, 2021

gaelicWizard commented Oct 12, 2021

davidpfarrell commented Oct 27, 2021

gaelicWizard commented Oct 28, 2021

davidpfarrell left a comment

Choose a reason for hiding this comment

NoahGorny left a comment

Choose a reason for hiding this comment

gaelicWizard commented Dec 27, 2021

Refactor completion for `defaults` command #1928

Refactor completion for `defaults` command #1928

gaelicWizard commented Sep 7, 2021 •

edited

Loading

gaelicWizard commented Sep 9, 2021 •

edited

Loading

davidpfarrell commented Sep 18, 2021 •

edited

Loading