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

Adds support for tab-completion of all parameters, long and short #395

Merged
merged 26 commits into from
Feb 6, 2017

Conversation

seamlessintegrations
Copy link
Contributor

@seamlessintegrations seamlessintegrations commented Feb 2, 2017

This fork adds support for tab-completing of nearly all parameters, short (-) and long (--), even with tab-completion of values.

Copy link
Collaborator

@rkeithhill rkeithhill left a comment

Choose a reason for hiding this comment

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

We have started adding Pester tests for TabExpansion. Could you add some Pester tests for this functionality? Look at test/TabExpansion.Tests.ps1 for examples of how to do this.

function script:expandParams($cmd, $filter) {
$params[$cmd] -split ' ' |
where { $_ -like "$filter*" } |
sort |
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please replace all aliases with the full command name: where -> where-object, sort -> sort-object, foreach -> foreach-object. We have folks running this on PowerShell Core on Linux and sort is a native command (not an alias for Sort-Object).

status = @{
'untracked-files' = 'no normal all'
'ignore-submodules' = 'none untracked dirty all' }
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

EOL at EOF please. We have a .editorconfig file that will handle this if you use an editor that works with EditorConfig files. For instance, Visual Studio Code.

@@ -17,6 +17,7 @@ if ($psv.Major -lt 3 -and !$NoVersionWarn) {
. $PSScriptRoot\GitUtils.ps1
. $PSScriptRoot\GitPrompt.ps1
. $PSScriptRoot\GitTabExpansion.ps1
. $PSScriptRoot\ParamsExpansions.ps1
Copy link
Collaborator

Choose a reason for hiding this comment

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

How about renaming this file to GitParamTabExpansion.ps1 (no need for the s on Params or Expansions). I think that would be consistent with most of the other scripts. Thanks!

@rkeithhill
Copy link
Collaborator

BTW I suspect this PR will need to undo or tweak what I did in PR #379. In that PR, I updated regex to ignore parameter between the remote and multiple refspec paths.

@@ -17,7 +17,7 @@ if ($psv.Major -lt 3 -and !$NoVersionWarn) {
. $PSScriptRoot\GitUtils.ps1
. $PSScriptRoot\GitPrompt.ps1
. $PSScriptRoot\GitTabExpansion.ps1
. $PSScriptRoot\ParamsExpansions.ps1
. $PSScriptRoot\GitParamsExpansion.ps1
Copy link
Collaborator

Choose a reason for hiding this comment

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

Sorry for being a bit pedantic here, but could you make that GitParamTabExpansion.ps1. Thanks.

@rkeithhill
Copy link
Collaborator

rkeithhill commented Feb 2, 2017

@seamlessintegrations Do you have a good debugger setup? If not, I'd recommend grabbing VS Code http://code.visualstudio.com. Then use Visual Studio Code to open your cloned posh-git folder. Open your Pester tests file and VS Code should prompt you to install the PowerShell extension. Once that is installed it will ask you to reload so the extension can be started. Once that is done, set a breakpoint in your test script, open the Debug view (View | Debug) and select "Launch Pester Tests" from the launch configuration drop down. Now press F5 and you'll be debugging your Pester tests.

Also, you might want to load the "EditorConfig for VS Code" extension. It will use the settings in that .editorconfig file.

@seamlessintegrations
Copy link
Contributor Author

@rkeithhill Thanks alot for your hints. I was just using powershell and vim / notepad++.

Regarding PR #379 I'm not quite sure what you mean. the gitParams regex works just fine with my params tab completion additions, and I can't see for what reasons they shouldn't.

@rkeithhill
Copy link
Collaborator

Does param completion work for fetch/push/pull? If so, then I guess we're good.

@rkeithhill
Copy link
Collaborator

Nevermind. I see you have tests specifically for push. I want to get @dahlbyk to review this as well.

@@ -0,0 +1,45 @@
. $PSScriptRoot\Shared.ps1
Copy link
Collaborator

@rkeithhill rkeithhill Feb 3, 2017

Choose a reason for hiding this comment

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

Can you rename your Pester test file to GitParamTabExpansion.Tests.ps1. We have a fledgling conventions of <filename-under-test>.Tests.ps1. Thanks.

@seamlessintegrations
Copy link
Contributor Author

seamlessintegrations commented Feb 3, 2017

I have tests only for push because this covers all functions that I've written. I can add more if you want.

Param Completion works for add, bisect, blame, branch, checkout, clean, clone, commit, config, describe, diff, difftool, fetch, gc, grep, help, init, log, merge, mergetool, mv, notes, prune, pull, push, rebase, reflog, remote, reset, revert, rm, shortlog, show, stash, status, submodule, tag and whatchanged. So yes, it works for fetch/push/pull.

Even parameter values can be completed or cycled through by repeatedly pressing TAB, e.g.

git pull --rebase=TAB => false
git pull --rebase=TAB => preserve
git pull --rebase=TAB => true

Copy link
Owner

@dahlbyk dahlbyk left a comment

Choose a reason for hiding this comment

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

This is awesome! Many many thanks for taking the initiative on this!

@@ -330,6 +351,21 @@ function GitTabExpansionInternal($lastBlock) {
gitBranches $matches['ref'] $true
gitTags $matches['ref']
}

# Handles git <cmd> --<param>=<value>
"^(?<cmd>$($someCommands -join '|')).* --(?<param>[^=]+)=(?<value>\S*)$" {
Copy link
Owner

Choose a reason for hiding this comment

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

I'm not terribly concerned about performance, but can we precompute $someCommands -join '|' since it's used a couple times?

More generally, It seems a little curious to use $someCommands here rather than explicitly looking for the keys in $paramvalues (or $params/$shortparams). Does everything still work as expected if a command is found in $someCommands but not one of the params lists? If not, we might consider a test to iterate over $someCommands (via tab-completing git <tab>) checking if long and short parameters can be completed successfully.

$params = @{
add = 'dry-run verbose force interactive patch edit update all no-ignore-removal no-all ignore-removal intent-to-add refresh ignore-errors ignore-missing'
bisect = 'no-checkout term-old term-new'
blame = 'root show-stats reverse porcelain line-porcelain incremental encoding= contents date score-debug show-name show-number show-email abbrev'
Copy link
Owner

Choose a reason for hiding this comment

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

tab? spaces!

encoding = 'utf-8' }
status = @{
'untracked-files' = 'no normal all'
'ignore-submodules' = 'none untracked dirty all' }
Copy link
Owner

Choose a reason for hiding this comment

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

Formatting nitpick - can you move the } on these definitions to its own line? If we need to add a parameter, diffs like this are a pet peeve:

-        'ignore-submodules' = 'none untracked dirty all' }
+        'ignore-submodules' = 'none untracked dirty all'
+        'insects' = 'ant butterfly' }

vs

         'ignore-submodules' = 'none untracked dirty all'
+        'insects' = 'ant butterfly'
         }

@rkeithhill
Copy link
Collaborator

Yeah, pretty awesome indeed. I'm looking forward to this functionality!

Also added cached param to diff.  That param should be there, right?  I mean, I use git diff --cached quite a bit.
@rkeithhill
Copy link
Collaborator

rkeithhill commented Feb 4, 2017

@dahlbyk Can we approve this? I have a PR queued up that fixes each of the issues you've raised. Also the cached parameter seems to be missing from the git diff command. It should be there, right?

Also, any chance you'd enable "squash & merge" on this repo - at least temporarily? I think this PR would be a good candidate for that.

Update: I submitted a PR to @seamlessintegrations fork. Hopefully, he'll have time shortly to look at it and merge it. I believe it should address all of @dahlbyk review issues.

@rkeithhill rkeithhill added this to the v0.7 milestone Feb 4, 2017
@seamlessintegrations
Copy link
Contributor Author

I'm sorry. I haven't seen your PR until now. I'll merge it soon.

@rkeithhill
Copy link
Collaborator

LGTM!

@dahlbyk dahlbyk merged commit 1327b98 into dahlbyk:master Feb 6, 2017
@dahlbyk
Copy link
Owner

dahlbyk commented Feb 6, 2017

Also, any chance you'd enable "squash & merge" on this repo - at least temporarily? I think this PR would be a good candidate for that.

I'm philosophically opposed to "squash & merge" because it loses the association between the PR commits and it coming into master:

* 7839e32 (HEAD -> master) Merge branch 'feature'
| * a3d6c38 (feature) Commit G
| * 1897f3d Commit F
| * 32bc8c4 Commit E
| * 510d269 Commit D
| * 7374aa9 Commit C
| * 8ec9109 Commit B
| * 2d02a6d Commit A
|/
* 9ca3d76

If we wanted to squash the commits, we should do it here in the PR branch. But history here is already rather messy anyway from merging year-old PRs - this is no exception.


Thanks again, @seamlessintegrations!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants