-
-
Notifications
You must be signed in to change notification settings - Fork 809
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
Fix missing CHERRY-PICKING, MERGING, REVERTING, etc #894
Conversation
This reverts commit f5dc3a2.
@NihilityT your fix does work, but I ended up reverting it to gain some performance. Not using the So the logic ends up being:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A few style comments but otherwise LGTM
@@ -59,7 +59,7 @@ function Get-GitDirectory { | |||
} | |||
} | |||
|
|||
function Get-GitBranch($gitDir = $(Get-GitDirectory), [Diagnostics.Stopwatch]$sw) { | |||
function Get-GitBranch($branch = $null, $gitDir = $(Get-GitDirectory), [switch]$isDotGitOrBare, [Diagnostics.Stopwatch]$sw) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is not an exported command but, in general, I'd add new parameters at the end of the parameter list to avoid breaking any callers that use positional parameters. That said, the only caller is in this file - besides to Pester test calls to this command. So, if it makes more sense for the Branch
parameter to be first then go for it. In retrospect, we probably should have been more consistent in using PascalCasing on all function parameters given that they correspond to command parameters e.g.: Get-GitBranch -Branch main -GitDir ~\.git
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is not an exported command but, in general, I'd add new parameters at the end of the parameter list to avoid breaking any callers that use positional parameters. That said, the only caller is in this file - besides to Pester test calls to this command. So, if it makes more sense for the
Branch
parameter to be first then go for it.
I generally agree, but I wanted to keep $sw
last so figured all bets are off.
In retrospect, we probably should have been more consistent in using PascalCasing on all function parameters given that they correspond to command parameters e.g.:
Get-GitBranch -Branch main -GitDir ~\.git
.
Consistency would be good. Can revisit holistically after I get a release out.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, it would be better to have style changes and semantic changes in separate PRs. Makes them both easier to review. 😁
$b = Invoke-NullCoalescing ` | ||
$b ` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Huh, I never realized until now that our Inovke-NullCoalescing
command handles an infinite list of args. Nice. The new PowerShell ??
operator allows this but not via a single operator e.g. $b = $b ?? $branch ?? { dbg...; git ... HEAD -q } ?? ...
And the ??
operator is PS checks for equality to $null
and not "truthiness" which can be surprising. And since the PS ??
operator is not in Windows PS 5, we can't use it here.
2db6914
to
66efbc4
Compare
Adding tests for #828