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

Initial support for vsts-cli #581

Merged
merged 6 commits into from
May 23, 2018
Merged

Initial support for vsts-cli #581

merged 6 commits into from
May 23, 2018

Conversation

flcdrg
Copy link
Contributor

@flcdrg flcdrg commented May 19, 2018

  • Add all pr commands and short commands (equivalent to vsts code pr)
  • Add test for filtering params

Implements #549

Not implemented in this pull request (future work):

  • 'subgroups' (work-items set-vote policies)
  • other vsts code operations (eg. repo)

@dahlbyk
Copy link
Owner

dahlbyk commented May 19, 2018

Neat! It's not obvious why builds are failing; do they pass on your machine?

@flcdrg
Copy link
Contributor Author

flcdrg commented May 19, 2018

They did.

I liked the fact that you had Pester there (I should make more use of it elsewhere!)

@flcdrg
Copy link
Contributor Author

flcdrg commented May 19, 2018

Hmm.. tests definitely failing on those builds though. I'll see if I can reproduce the problem locally. Is there any tricks to testing posh-git?

@flcdrg
Copy link
Contributor Author

flcdrg commented May 19, 2018

For some reason, on the CI builds, the $result is empty for my new tests, but not having any success replicating that locally

@flcdrg
Copy link
Contributor Author

flcdrg commented May 19, 2018

Looks like it's related to the alias that vsts-cli creates for git. Will need to mock something for the tests..

- Add all pr commands and short commands
- Add test for filtering params

Implements dahlbyk#549

Not implemented in this PR: 'subgroups' (work-items set-vote policies)
The idea "known alias" is that we can standardize handling of a
particular alias command (e.g. 'git pr' pass-through to 'vsts code pr')
without hooking on the alias name. For example, 'pr' could reasonably be
used to manage GitHub, so this allows us to avoid using vsts tab
expansion in that case.

Namespacing the alias ('vsts.pr') prevents conflicts with a real alias.
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.

In retrospect I probably should have PR'd into your PR, since this is more of a proposal than a fix, but too late for that.

Anyway, as I noted in the commit I'm trying out the idea of shifting this to a "known alias":

The idea "known alias" is that we can standardize handling of a
particular alias command (e.g. 'git pr' pass-through to 'vsts code pr')
without hooking on the alias name. For example, 'pr' could reasonably be
used to manage GitHub, so this allows us to avoid using vsts tab
expansion in that case.

Namespacing the alias ('vsts.pr') prevents conflicts with a real alias.

Rather than matching what the VSTS pr alias expands into, the "known alias" idea allows us to swap in a "command" that posh-git knows how to match whenever it recognizes the alias value. To prove the point, I've shifted the tests to not actually depend on a pr alias, just an alias that acts like it. Thoughts?

More generally, I'm not thrilled with the hoops you've had to jump through to get sub-command expansion working. I believe git flow is the only other place we're doing subcommand expansion, but without parameters. I'd love to be able to do something like $longGitParams['vsts.pr'] = $longVstsParams and have all the subcommands work as expected without our remaining vsts.pr-specific matches. But we don't have to do that here before merging, just dreaming.

@dahlbyk dahlbyk requested a review from rkeithhill May 19, 2018 22:53
@flcdrg
Copy link
Contributor Author

flcdrg commented May 20, 2018

Totally fine with your changes 😃

I tried to fit in with the existing structure and make the smallest change possible to get the subcommands working as I could.

Making a more general solution totally makes sense. I'd guess the design for that could start to become evident once there's one or two more extensions that want to be included.

One of the areas I skipped for this PR was the vsts 'subgroup' commands, which I think end up a bit like sub-sub-sub commands.

Another thing for the future is to add value completion for the boolean and 'enum' type parameters.

But baby steps - get something basic working and then I can build on that!

@dahlbyk
Copy link
Owner

dahlbyk commented May 20, 2018

Making a more general solution totally makes sense. I'd guess the design for that could start to become evident once there's one or two more extensions that want to be included.

It's an interesting design process, for sure. I'd almost argue that posh-git isn't even the right place to solve this problem for vsts. Maybe better to hook into a posh-vsts™ module's Expand-VstsCommand for relevant aliases.

Out of curiosity, is there any overlap between data available from vsts and opportunities to improve git-tfs expansion support? I don't use either.

But baby steps - get something basic working and then I can build on that!

💯

I'm going to give this a day or two to see if @rkeithhill or anyone else weighs in.

@@ -264,6 +276,23 @@ function GitTabExpansionInternal($lastBlock, $GitStatus = $null) {

switch -regex ($lastBlock -replace "^$(Get-AliasPattern git) ","") {

# Handles git pr alias
Copy link
Collaborator

Choose a reason for hiding this comment

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

It's not a big perf hit but it strikes me that these "case" statements are less likely to be encountered than the basic git <cmd> completions. As such, it seems like the new vsts-oriented "case" statements should go towards the bottom after the more common cases.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Other than the above, this PR LGTM.

@flcdrg
Copy link
Contributor Author

flcdrg commented May 22, 2018

Out of curiosity, is there any overlap between data available from vsts and opportunities to improve git-tfs expansion support? I don't use either.

I'm not sure. git-tfs is a bridge like git-svn. I used it a few years back against an on-prem TFS server. It doesn't have the concept of pull requests, so I don't think there's much in common. It seems the focus of vsts-cli is git operations rather than TFVC.

- Following @rkeithhill's suggestion
@dahlbyk dahlbyk merged commit b428c3c into dahlbyk:master May 23, 2018
@dahlbyk
Copy link
Owner

dahlbyk commented May 23, 2018

Thanks!

1 similar comment
@rkeithhill
Copy link
Collaborator

Thanks!

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.

3 participants