4 ms·

How We Use Pull Requests To Build GitHub

45 points by pathdependent 14y ago

buss 14y ago
Pull requests are great, but I think they don't work well for the example that github provides. This use case sucks for reviewers: 1. push commits to a branch & open a pull request 2. get a bunch of comments 3. push a few more commits fixing some errors 4. more comments 5. more fixes At every step of this process you either have to review each individual commit or the entire diff. You can't see only what's changed between two arbitrary commits. I endure this process so often (and it makes me pine for review board) that I wrote a greasemonkey script to view intermediate diffs: https://github.com/sbuss/pull_request_intermediate_diffs https://github.com/sbuss/pull_request_intermediate_diffs Github, apparently, isn't interested in fixing this (https://github.com/blog/1124-how-we-use-pull-requests-to-build-github#comment-19295 https://github.com/blog/1124-how-we-use-pull-requests-to-bui...)
- tomblomfield 14y ago
  I think pull requests are awesome, especially where you're making a 5-20 line change on some really important bit of your site. It's really simple to see what's changed and have colleagues do code-review really quickly. But when you're implementing a major feature, it falls apart. Today, I had to code-review a pull request with 195 separate commits, a 3000-line diff and a combination of css, coffeescript & ruby. Pull requests become pretty useless at this point. Sure, you can do intermediary pull-requests, but at the end you're left with this huge beomoth and no idea where to start. Intermediary diffs could be the answer. Perhaps also the ability to filter the pull request (eg. don't show me the CSS)
  - briandoll 14y ago
    Another great part of using pull requests, is that they allow everyone to see what's going on, mid-stream. They are awesome for major features, especially if you open them early on. We all collaborate via pull requests as various features are being worked on. This way, when someone leaves a comment asking about the overall design (code or visual), suggesting a different approach, it's done while the feature is in development, not when the author thinks the feature is ready to ship. Doing a big code review after a feature has been completely built and is ready to ship seems like a mini-waterfall process.
- ajross 14y ago
  Honestly, making fixes for review comment separate commits is bad practice. Long term, what you want is for the history of the project to be a long chain of individually justified changes that do something "real" with the software. What you don't want is a bunch of churn like "Fix whitespace per buss's comments" or "Use correct naming scheme". And what you really don't want is for some of those commits to be broken: to the extent practical all commits should be testable so that tools like "git bisect" will work. So the proper behavior for someone submitting a pull request is to rework the whole thing and submit it again, not patch on top of the original. Check how the kernel works for a great example of this.

45 points by pathdependent 14y ago

buss 14y ago

Pull requests are great, but I think they don't work well for the example that github provides. This use case sucks for reviewers: 1. push commits to a branch & open a pull request 2. get a bunch of comments 3. push a few more commits fixing some errors 4. more comments 5. more fixes At every step of this process you either have to review each individual commit or the entire diff. You can't see only what's changed between two arbitrary commits. I endure this process so often (and it makes me pine for review board) that I wrote a greasemonkey script to view intermediate diffs: https://github.com/sbuss/pull_request_intermediate_diffs https://github.com/sbuss/pull_request_intermediate_diffs Github, apparently, isn't interested in fixing this (https://github.com/blog/1124-how-we-use-pull-requests-to-build-github#comment-19295 https://github.com/blog/1124-how-we-use-pull-requests-to-bui...)

tomblomfield 14y ago
I think pull requests are awesome, especially where you're making a 5-20 line change on some really important bit of your site. It's really simple to see what's changed and have colleagues do code-review really quickly. But when you're implementing a major feature, it falls apart. Today, I had to code-review a pull request with 195 separate commits, a 3000-line diff and a combination of css, coffeescript & ruby. Pull requests become pretty useless at this point. Sure, you can do intermediary pull-requests, but at the end you're left with this huge beomoth and no idea where to start. Intermediary diffs could be the answer. Perhaps also the ability to filter the pull request (eg. don't show me the CSS)
- briandoll 14y ago
  Another great part of using pull requests, is that they allow everyone to see what's going on, mid-stream. They are awesome for major features, especially if you open them early on. We all collaborate via pull requests as various features are being worked on. This way, when someone leaves a comment asking about the overall design (code or visual), suggesting a different approach, it's done while the feature is in development, not when the author thinks the feature is ready to ship. Doing a big code review after a feature has been completely built and is ready to ship seems like a mini-waterfall process.
ajross 14y ago
Honestly, making fixes for review comment separate commits is bad practice. Long term, what you want is for the history of the project to be a long chain of individually justified changes that do something "real" with the software. What you don't want is a bunch of churn like "Fix whitespace per buss's comments" or "Use correct naming scheme". And what you really don't want is for some of those commits to be broken: to the extent practical all commits should be testable so that tools like "git bisect" will work. So the proper behavior for someone submitting a pull request is to rework the whole thing and submit it again, not patch on top of the original. Check how the kernel works for a great example of this.