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

Repositories - Merging #84

Open
raghav710 opened this issue Jan 12, 2019 · 7 comments
Open

Repositories - Merging #84

raghav710 opened this issue Jan 12, 2019 · 7 comments
Assignees
Labels
api completeness This is basic API functionality that hasn't been implemented yet. enhancement An issue or pull request introducing new functionality to the project.

Comments

@raghav710
Copy link

raghav710 commented Jan 12, 2019

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
Copy link
Author

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

@HowardWolosky HowardWolosky added enhancement An issue or pull request introducing new functionality to the project. api completeness This is basic API functionality that hasn't been implemented yet. labels Jan 12, 2019
@HowardWolosky
Copy link
Member

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.

@raghav710
Copy link
Author

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

@HowardWolosky
Copy link
Member

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.

@raghav710
Copy link
Author

@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
Copy link
Author

Also, here is a tentative PR: #89
I've tested the code manually using my own access token

@HowardWolosky
Copy link
Member

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api completeness This is basic API functionality that hasn't been implemented yet. enhancement An issue or pull request introducing new functionality to the project.
Projects
None yet
Development

No branches or pull requests

2 participants