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

Merge a branch of a repository #89

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

raghav710
Copy link

Does not contain tests yet

Copy link
Member

@HowardWolosky HowardWolosky left a comment

Choose a reason for hiding this comment

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

Thanks for the submission. You're off to a great start. There are some areas to improve, but this is looking fairly solid. Much appreciated!

GitHubRepositoryMerge.ps1 Outdated Show resolved Hide resolved
GitHubRepositoryMerge.ps1 Outdated Show resolved Hide resolved
GitHubRepositoryMerge.ps1 Show resolved Hide resolved
GitHubRepositoryMerge.ps1 Show resolved Hide resolved
GitHubRepositoryMerge.ps1 Show resolved Hide resolved
GitHubRepositoryMerge.ps1 Outdated Show resolved Hide resolved
GitHubRepositoryMerge.ps1 Outdated Show resolved Hide resolved
GitHubRepositoryMerge.ps1 Outdated Show resolved Hide resolved
GitHubRepositoryMerge.ps1 Outdated Show resolved Hide resolved
GitHubRepositoryMerge.ps1 Outdated Show resolved Hide resolved
@HowardWolosky
Copy link
Member

Hi @raghav710 -- just reaching out. Are you still working on this?

@raghav710
Copy link
Author

@HowardWolosky sorry something came up and I didnt devote my time to this. I am planning to get this done with some form of test cases this weekend. Would that work fine?

@HowardWolosky
Copy link
Member

@raghav710 Not a problem. I just needed to know if this was abandoned. If you're still working on it, please continue to do so at times that are convenient to you. I appreciate the contribution.

@@ -0,0 +1,219 @@
# Copyright (c) Microsoft Corporation. All rights reserved.
Copy link
Author

Choose a reason for hiding this comment

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

Please ignore this file, I'll remove the associated commit and rebase/squash as required after the GitHubReference changes are in

@@ -0,0 +1,173 @@
# Copyright (c) Microsoft Corporation. All rights reserved.
Copy link
Author

Choose a reason for hiding this comment

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

Please ignore this file

@raghav710
Copy link
Author

@HowardWolosky I have added a set of test cases
I realized I need the functionality to create content in a branch as that will be useful to test the merging and merge conflict related scenarios. You will find GitHubReference related changes in this branch. I'll remove these changes before merging.

@HowardWolosky HowardWolosky self-assigned this Mar 26, 2019
Copy link
Member

@HowardWolosky HowardWolosky left a comment

Choose a reason for hiding this comment

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

Hey there,

Again, thanks for the updates, and my apologies for missing that you had actually submitted an update to the PR that I completely missed (doing some project house-cleaning today and noticed that there were these unaddressed PR's).

It looks like there are some minor PR changes being requested here, and some merging/resolving to handle, as well as the fact that you have some of the References stuff in this PR as well. I'll await a further update from you before digging further into this one too deeply.

Thanks!

Tests/GitHubRepositoryMerge.tests.ps1 Show resolved Hide resolved
GitHubReferences.ps1 Show resolved Hide resolved
GitHubReferences.ps1 Show resolved Hide resolved
GitHubRepositoryMerge.ps1 Show resolved Hide resolved
GitHubReferences.ps1 Show resolved Hide resolved
return $result.result
}
catch {
#TODO: Read the error message and find out the kind of issue
Copy link
Member

Choose a reason for hiding this comment

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

What's your plan here?

Copy link
Author

Choose a reason for hiding this comment

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

The idea was to see if we could extract the status code and give a more informative message to the user. But looks like that would require parsing of the string that we get as an exception which doesn't sound clean to me. I'm leaving this as-is for now. But please do let me know your thoughts on if this can be handled better

@raghav710
Copy link
Author

@HowardWolosky thanks for the comments. I'll take stock of the changes (It's been some time since I had a look at the code) and respond to the comments

@HowardWolosky HowardWolosky added the api completeness This is basic API functionality that hasn't been implemented yet. label May 22, 2020
@HowardWolosky HowardWolosky added the waiting for update Waiting for an update to the PR before the next review label Jun 20, 2020
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. waiting for update Waiting for an update to the PR before the next review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants