ghi
ghi copied to clipboard
stephencelis
Add support for pull requests
The code belongs to @LFDM. I just rebased it to master, fixing conflicts. The pull request #158 was closed because of merge conflicts. Fixes issue #256. Because of other reservations I was not able to test it extensively, but with basic testing its working. I would be grateful if someone can test it. You can test it using the link https://raw.githubusercontent.com/shubhamshuklaer/ghi/pulls_fix/ghi .
Hi @shubhamshuklaer!
~~Thanks for the PR, would you mind squashing all of the commits into one please?~~
Thanks!
Obviously as this is a substantial PR I don't feel I have the ability to properly review it, I'll have to leave that down to @stephencelis
I might not have time to look over this big of a diff for awhile. From what I remember, @LFDM's pull was great, but I encountered a few minor bugs. These cases may have been unrelated and subsequently fixed, though. I think stress testing this PR will give us some confidence. I encourage all ghi users interested in PR support to pull down this version and test it within their normal workflow, reporting bugs (or lack thereof) back here.
@shubhamshuklaer (and anyone else who's following this branch) Have you been running this exclusively and found things to be relatively stable?
Well I don't get to use pull request that often as I don't manage any big repo. I will see if I can test it this week.
Though the tests in #308 are not very exhaustive(We need more tests). But I tested by creating a branch off of this(#288) and then merging #308 into it. All test passed. https://travis-ci.org/shubhamshuklaer/ghi/builds/133670481 https://github.com/shubhamshuklaer/ghi/tree/pulls_travis_tmp
Though #308 doesn't contain any test for pull request. But at-least it means it won't(most probably, need more tests) cause a regression.
Once #308 is merged, I will try adding tests for this by rebasing it off of master.
I unfortunately don't have the bandwidth to manage this right now. Feel free to move ahead with thorough testing, but be mindful of our users before release!
@timofonic Do you know if it has been tested? I'm not good enough with Ruby to blindly merge it into my fork.
