-
Notifications
You must be signed in to change notification settings - Fork 189
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
base: master
Are you sure you want to change the base?
Conversation
There was a problem hiding this 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!
Hi @raghav710 -- just reaching out. Are you still working on this? |
@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? |
@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. |
…mented * Includes tests for a few common scenarios * Contains test cases
…0/PowerShellForGitHub into issue#84-merge-branches
@@ -0,0 +1,219 @@ | |||
# Copyright (c) Microsoft Corporation. All rights reserved. |
There was a problem hiding this comment.
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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please ignore this file
@HowardWolosky I have added a set of test cases |
There was a problem hiding this 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!
return $result.result | ||
} | ||
catch { | ||
#TODO: Read the error message and find out the kind of issue |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
@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 |
Does not contain tests yet