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

feature/tab-expand-pwsh-alias #779

Merged
merged 31 commits into from Sep 12, 2020

Conversation

csc027
Copy link
Contributor

@csc027 csc027 commented Jun 22, 2020

This patch implements what @jkymarsh wanted in #257. I also included changes to handle the newer expansion logic with Register-ArgumentCompleter. Many of the basic test cases for the proxy commands have already been implemented, but I think I may be missing some edge cases here. In broader terms, what am I missing in terms of test cases?

There is also the issue of the regular expression not not handling all whitespace characters. It currently handles the case of multiline commands. For example:

function Test {
    git `
    checkout `
    $args
}

The original regular expression used the \s character class,

"(^|[|;])\s*(?<cmd>$(Get-AliasPattern git).* \`$args)\s*($|[|;])"

but incorrectly allowed multiline commands without the backticks.

The regular expression for reference:

# The regular expression here is roughly follows this pattern:
#
# <begin anchor><whitespace>*<git>(<whitespace><parameter>)*<whitespace>+<$args><whitespace>*<end anchor>
#
# The delimiters inside the parameter list and between some of the elements are non-newline whitespace characters ([^\S\r\n]).
# In those instances, newlines are only allowed if they preceded by a non-newline whitespace character.
#
# Begin anchor (^|[;`n])
# Whitespace   (\s*)
# Git Command  (?<cmd>$(GetAliasPattern git))
# Parameters   (?<params>(([^\S\r\n]|[^\S\r\n]``\r?\n)+\S+)*)
# $args Anchor (([^\S\r\n]|[^\S\r\n]``\r?\n)+\`$args)
# Whitespace   (\s|``\r?\n)*
# End Anchor   ($|[|;`n])
$script:GitProxyCommandRegex = "(^|[;`n])(\s*)(?<cmd>$(Get-AliasPattern git))(?<params>(([^\S\r\n]|[^\S\r\n]``\r?\n)+\S+)*)(([^\S\r\n]|[^\S\r\n]``\r?\n)+\`$args)(\s|``\r?\n)*($|[|;`n])"

csc027 added 16 commits June 21, 2020 18:59
…d proxy command names to the argument completer.
…s1 file to script scope. Separated the alias tests into their own context.
…tup and teardown sections for tests. Updated the alias to use script scope instead of global scope in the tests.
@csc027 csc027 marked this pull request as ready for review June 23, 2020 04:27
@csc027
Copy link
Contributor Author

csc027 commented Jun 24, 2020

Right now, there's an issue with how the proxy command expansion works in the PowerShell 6+ tab expansion implementation. Because the registration for the proxy commands needs to happen at module load time, any changes to the $Global:GitTabSettings object after the module load will be ineffective. There are two ways I can see to solve this:

  1. Add a function that can register the proxy commands after module load.
  2. Change the way the settings object is generated (i.e., do not overwrite an existing object and allow users to enable settings before the module is loaded).

I'm leaning towards the first option, but I realize it isn't very idiomatic for the module to configure settings via a function.


Edit: Didn't see that you loaded the $UseLegacyTabExpansion variable in the argument list to use it at module load time. I've switched to using this solution.

@MisinformedDNA
Copy link

@dahlbyk Where are we with this?

@csc027
Copy link
Contributor Author

csc027 commented Aug 6, 2020

@MisinformedDNA I've been using this implementation for about six weeks on Windows with Powershell 7, and I haven't encountered any issues yet. It may fail on some cases I haven't encountered, but it shouldn't be too hard to push fixes in with the tests I added.

Still waiting on the review though.

@rkeithhill
Copy link
Collaborator

Since this isn't enabled by default, I think we should add this for v1 and have folks try it out. I'll be adding some review comments here shortly mostly about style nits. After those are addressed, I'll accept this PR.

src/GitTabExpansion.ps1 Outdated Show resolved Hide resolved
src/GitTabExpansion.ps1 Outdated Show resolved Hide resolved
src/GitTabExpansion.ps1 Outdated Show resolved Hide resolved
src/GitTabExpansion.ps1 Outdated Show resolved Hide resolved
src/GitTabExpansion.ps1 Outdated Show resolved Hide resolved
src/GitTabExpansion.ps1 Outdated Show resolved Hide resolved
src/GitTabExpansion.ps1 Outdated Show resolved Hide resolved
test/GitProxyCommandExpansion.Tests.ps1 Outdated Show resolved Hide resolved
src/GitTabExpansion.ps1 Outdated Show resolved Hide resolved
src/GitTabExpansion.ps1 Outdated Show resolved Hide resolved
src/posh-git.psm1 Outdated Show resolved Hide resolved
@rkeithhill rkeithhill merged commit e4231b8 into dahlbyk:master Sep 12, 2020
@csc027
Copy link
Contributor Author

csc027 commented Sep 12, 2020

Thanks @rkeithhill for reviewing. 😃

@MisinformedDNA
Copy link

@csc027 How can I enable this functionality?

@csc027
Copy link
Contributor Author

csc027 commented Jan 26, 2021

@MisinformedDNA If you have the version with the change in, you have to import while setting the module arguments. I don't think the PowerShell Gallery version has been updated with these changes yet; so, you'll likely need to import it from your Git repository directly. Either way, the module arguments are:

[bool]$ForcePoshGitPrompt,
[bool]$UseLegacyTabExpansion,
[bool]$EnableProxyFunctionExpansion

, which can be set via:

Import-Module -Name '<path to posh-git.psd1 file>' -ArgumentList @($false, $false, $true)

@MisinformedDNA
Copy link

so, you'll likely need to import it from your Git repository directly

Ah... that's what I was missing. Thanks!

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.

This LGTM. Only issue is that I think perhaps the regex should be moved to the GitTabSettings object so that folks have a chance to tweak the regex for their proxy functions.

src/GitTabExpansion.ps1 Show resolved Hide resolved
@rkeithhill
Copy link
Collaborator

Oops. This is already merged. Yikes this tab has been hanging around in my browser for a long time. :-)

hoang-himself added a commit to hoang-himself/dotfiles that referenced this pull request Aug 22, 2022
hoang-himself added a commit to hoang-himself/dotfiles that referenced this pull request Aug 25, 2022
hoang-himself added a commit to hoang-himself/dotfiles that referenced this pull request Aug 26, 2022
hoang-himself added a commit to hoang-himself/dotfiles that referenced this pull request Aug 27, 2022
@SB-USNC
Copy link

SB-USNC commented Sep 2, 2022

This seems to work on aliases but not on functions. For example:

function Get-GitSwitch { & git switch -v $args }
New-Alias -Name gw -Value Get-GitSwitch -Force -Option AllScope

gw doesn't provide a Menu Complete with Ctrl + Space

still have to write g switch

@kleinfreund
Copy link

@SB-USNC The tab completion should work for the function. What doesn't work here is the tab completion for the alias to the function.

I use a lot of functions defined like this and with this feature enabled, I get tab completion (and Ctrl+Space) when using gsw.

function gsw { git switch $args }

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.

None yet

5 participants