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

Add Update-VSTeamPullRequest and Add-VSTeamPullRequest #241

Merged
merged 15 commits into from
Feb 25, 2020
Merged

Add Update-VSTeamPullRequest and Add-VSTeamPullRequest #241

merged 15 commits into from
Feb 25, 2020

Conversation

MichelZ
Copy link
Contributor

@MichelZ MichelZ commented Feb 20, 2020

PR Summary

Add Update-VSTeamPullRequest
Add Add-VSTeamPullRequest
Fixes #120
Fixes #131

PR Checklist

@SebastianSchuetze SebastianSchuetze self-requested a review February 20, 2020 22:11
@SebastianSchuetze
Copy link
Collaborator

thx, gonna review this soon

@MichelZ
Copy link
Contributor Author

MichelZ commented Feb 21, 2020

I think CI builds fail not because of the changes here, but because of flawed tests.
Fixed in #242

Fix unit tests

Catch more instances to replace

Add initial draft of Update-VSTeamPullRequest

Add Documentation, Unit Tests, Refinements

Remove mixed-up branch changes

Remove mixed-up branch changes
@MichelZ MichelZ changed the title [WIP] Update pull request Add Update-VSTeamPullRequest and Add-VSTeamPullRequest Feb 21, 2020
@MichelZ MichelZ marked this pull request as ready for review February 21, 2020 11:14
@DarqueWarrior
Copy link
Collaborator

DarqueWarrior commented Feb 25, 2020

OK I have a few questions on this one. In Update-VSTeamPullRequest:
Why do we need both -Draft and -Publish? It appears that -Draft should be enough. If it is present you set the body to true if not to false. Why do I need them both?

Also the ProjectName parameter is never used and seems like it can be removed from Update-VSTeamPullRequest.

I also don't think we need a Draft ParameterSet. Just remove that ParameterSet and remove all set names from Draft and Force.

@DarqueWarrior
Copy link
Collaborator

I will make the changes I think need to be made and commit them so we can review.

@DarqueWarrior
Copy link
Collaborator

Hey @MichelZ and @SebastianSchuetze can you look at my changes and make sure I have not missed anything.

@DarqueWarrior DarqueWarrior merged commit 19632b2 into MethodsAndPractices:master Feb 25, 2020
@MichelZ
Copy link
Contributor Author

MichelZ commented Feb 25, 2020

@DarqueWarrior @SebastianSchuetze The reason that both are there: It's more explicit. e.g. if you update a draft to published, now you have to:
Update-VSTeamPullRequest -PullRequestId 1234
That doesn't seem descriptive or intuitive, where:
Update-VSTeamPullRequest -PullRequestId 1234 -Publish
feels very logical.

Also, if you want to e.g. update a draft pull request with anything, you have to think about including -Draft that it stays draft, else it gets automatically published. That screams "bug" to me :)

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.

[Feature] Add New-VSTeamPullRequest command Feature request: Ability to set Pull Request status
3 participants