Skip to content

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

Merged
dahlbyk merged 26 commits into
dahlbyk:masterfrom
seamlessintegrations:master
Feb 6, 2017
Merged

Adds support for tab-completion of all parameters, long and short#395
dahlbyk merged 26 commits into
dahlbyk:masterfrom
seamlessintegrations:master

Conversation

@seamlessintegrations

@seamlessintegrations seamlessintegrations commented Feb 2, 2017

Copy link
Copy Markdown
Contributor

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

@rkeithhill rkeithhill left a comment

Copy link
Copy Markdown
Collaborator

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.

Comment thread src/GitTabExpansion.ps1 Outdated
function script:expandParams($cmd, $filter) {
$params[$cmd] -split ' ' |
where { $_ -like "$filter*" } |
sort |

Copy link
Copy Markdown
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).

Comment thread src/ParamsExpansions.ps1
status = @{
'untracked-files' = 'no normal all'
'ignore-submodules' = 'none untracked dirty all' }
}

Copy link
Copy Markdown
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.

Comment thread src/posh-git.psm1 Outdated
. $PSScriptRoot\GitUtils.ps1
. $PSScriptRoot\GitPrompt.ps1
. $PSScriptRoot\GitTabExpansion.ps1
. $PSScriptRoot\ParamsExpansions.ps1

Copy link
Copy Markdown
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
Copy Markdown
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.

Comment thread src/posh-git.psm1 Outdated
. $PSScriptRoot\GitPrompt.ps1
. $PSScriptRoot\GitTabExpansion.ps1
. $PSScriptRoot\ParamsExpansions.ps1
. $PSScriptRoot\GitParamsExpansion.ps1

Copy link
Copy Markdown
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

rkeithhill commented Feb 2, 2017

Copy link
Copy Markdown
Collaborator

@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
Copy Markdown
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
Copy Markdown
Collaborator

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

@rkeithhill

Copy link
Copy Markdown
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

@rkeithhill rkeithhill Feb 3, 2017

Copy link
Copy Markdown
Collaborator

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

seamlessintegrations commented Feb 3, 2017

Copy link
Copy Markdown
Contributor Author

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

@dahlbyk dahlbyk left a comment

Copy link
Copy Markdown
Owner

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!

Comment thread src/GitTabExpansion.ps1 Outdated
}

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

Copy link
Copy Markdown
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.

Comment thread src/GitParamTabExpansion.ps1 Outdated
$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
Copy Markdown
Owner

Choose a reason for hiding this comment

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

tab? spaces!

Comment thread src/GitParamTabExpansion.ps1 Outdated
encoding = 'utf-8' }
status = @{
'untracked-files' = 'no normal all'
'ignore-submodules' = 'none untracked dirty all' }

Copy link
Copy Markdown
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
Copy Markdown
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

rkeithhill commented Feb 4, 2017

Copy link
Copy Markdown
Collaborator

@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
Copy Markdown
Contributor Author

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

@rkeithhill

Copy link
Copy Markdown
Collaborator

LGTM!

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

dahlbyk commented Feb 6, 2017

Copy link
Copy Markdown
Owner

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