PowerShellForGitHub icon indicating copy to clipboard operation
PowerShellForGitHub copied to clipboard

Repositories - Merging

Open raghav710 opened this issue 7 years ago • 7 comments

Issue to track the implementation of the APIs for "merging" under the "Repository" section in #70 API reference: https://developer.github.com/v3/repos/merging/

I will update this issue with my proposals for the cmdlet names and parameters

raghav710 avatar Jan 12 '19 10:01 raghav710

Here are my proposals for this cmdlet: Name: Merge-GitHubRepositoryBranches FileName: GitHubRepositoryMerge.ps1 It takes the following parameters

  • OwnerName
  • RepositoryName
  • Uri
  • AccessToken
  • NoStatus
  • Base (base in the API)
  • Head (head in the API)
  • CommitMessage (commit_message in the API)

Checklists

  • [ ] Test the cmdlet using my own access token
  • [ ] Add tests testing for different combinations of parameters

I have read the contributing.md and will try to follow the same. @HowardWolosky would like to know if there are anything additional I need to keep in mind before implementation

raghav710 avatar Jan 12 '19 11:01 raghav710

Thanks @raghav710! Overall, this looks great. Looking forward to having this functionality.

Some minor feedback here:

  • Please keep AccessToken and NoStatus as the last parameters in the method, just for consistency with the rest of the module.
  • You may want to call Invoke-GHRestMethod with the -ExtendedResult parameter so that you can detect a 201 vs a 204 and be able to do Write-Log -Level Warning in the event that it's a 204 explaining that there's nothing to merge.

HowardWolosky avatar Jan 12 '19 15:01 HowardWolosky

@HowardWolosky thanks for the note on the 201 vs 204. Looking forward to getting this in :)

raghav710 avatar Jan 15 '19 05:01 raghav710

Me too.

The test may be hard to write though because we don't yet have API support for creating new pull requests... Something to keep in mind.

HowardWolosky avatar Jan 15 '19 06:01 HowardWolosky

@HowardWolosky I think I need the ability to create branches so I can add tests for this. Shall I concentrate on adding that and then come back to this?

raghav710 avatar Feb 19 '19 11:02 raghav710

Also, here is a tentative PR: https://github.com/PowerShell/PowerShellForGitHub/pull/89 I've tested the code manually using my own access token

raghav710 avatar Feb 19 '19 11:02 raghav710

Thanks Raghav! I've given your tentative PR an initial code review. I do think that it would be best to only check in features that have corresponding tests that can help ensure that the module remains working as expected. If you're able to add the necessary, tested methods for creating branches so that you can add tests for this PR, that would be great.

I think you may run into some trouble writing tests for the refs API's too though, because per the documentation, you can't create new refs on empty repos...so, you'd have to make sure that it has content in it first. It's possible that using -AutoInit to create it with an empty README will be sufficient to satisfy that requirement however.

HowardWolosky avatar Feb 20 '19 22:02 HowardWolosky