I can create a PR, get it reviewed / approved, then change 100% of the underlying code and still merge it.
Having required reviews is a step in the right direction, but not quite good enough to work as a proper code review / signoff for strict compliance requirements.
Could you expand on your use-case? I'm unfamiliar with this kind of scenario and I'd like to learn more about your constraints and their reason.
Maybe I'm being completely naïve here, but it would seem like anyone with very strict compliance requirements wouldn't be using GitHub in the first place. Is this line of reasoning wrong?
At least for my small (5-ish people) team, it's been great. It makes it much easier to communicate the current status. But since we're such a small team, we largely just trust everyone to be careful. We generally don't commit directly to master and all PRs are reviewed. But if there's a good reason to commit directly or immediately merge a PR without review, we're fine with that. We'll usually just notify the team on Slack and they'll find out in the morning. For example: we get a bug report and there's a quick fix, but nobody is around to review it. The developer is encouraged to merge, confirm it's working on staging, and release it.
Being able to release multiple times per day might not seem like a big deal, but it means you start releasing smaller changes with increased frequency, so even if something goes wrong, you can easily revert and you know exactly what to investigate.
Honestly, just being able to "tick" each commit would be great.
On https://github.com/bitcoin/bitcoin/ it is common practice to include the commit hash in your approval comment so you can easily review where you were up to if a rebase occurs.
* can't initiate a review from any tab
* can't quickly give an official approval
* have to decide whether comments are in a review or not
I'd like to comment on the commit and commit messages directly (eg, "Please rebase out the WIP comments here")
There's lots of friction when looking anywhere ELSE in the code during a code review. I'd like to be able to make a comment on eg, a line of code that needed to be changed in a file not in the review.
Github PRs are getting much better, and that's good!
One thing missing for me now is an interdiff feature. E.g. show me the diff minus <some initial commit that's a big vendor commit>.
@<username> is a pretty common pattern for mentioning people for reviews right now. Any chance we get a text shortcut for adding reviewers? Not strictly necessary but a nice-to-have
It's a great feature, although the UX for packaging several comments into one or multiple reviews, each with their own status/requirement feels a little over the top. Even later, it's comments interspersed through the conversation, instead of being able to view and take action on a single review as a unit.
so true, it lacks an inbox feature like Bitbucket Server (Atlassian Stash) has. On top of that:
- keeping track of changes over the life time of a PR is impossible, you have review the whole thing over and over again
- toggle notes (comments) on a per file basis is silly and unpractical on PR's with lots of files and comments.
- there is no way to see which PR from a list you already approved.
And the list goes on and on. I have been using Github Enterprise at my new job since recently and it is by far the poorest of tools I have ever used to review code.
[+] [-] web007|9 years ago|reply
Having required reviews is a step in the right direction, but not quite good enough to work as a proper code review / signoff for strict compliance requirements.
[+] [-] TheAceOfHearts|9 years ago|reply
Maybe I'm being completely naïve here, but it would seem like anyone with very strict compliance requirements wouldn't be using GitHub in the first place. Is this line of reasoning wrong?
At least for my small (5-ish people) team, it's been great. It makes it much easier to communicate the current status. But since we're such a small team, we largely just trust everyone to be careful. We generally don't commit directly to master and all PRs are reviewed. But if there's a good reason to commit directly or immediately merge a PR without review, we're fine with that. We'll usually just notify the team on Slack and they'll find out in the morning. For example: we get a bug report and there's a quick fix, but nobody is around to review it. The developer is encouraged to merge, confirm it's working on staging, and release it.
Being able to release multiple times per day might not seem like a big deal, but it means you start releasing smaller changes with increased frequency, so even if something goes wrong, you can easily revert and you know exactly what to investigate.
[+] [-] RSZC|9 years ago|reply
[+] [-] hhjkjhkjhhlkj|9 years ago|reply
Still it is a start, and will hopefully improve.
[+] [-] dcousens|9 years ago|reply
[+] [-] rcaught|9 years ago|reply
[+] [-] Pirate-of-SV|9 years ago|reply
Or something to enforce: "One person creates pull request, another reviews, a third merges."
[+] [-] tln|9 years ago|reply
* can't initiate a review from any tab * can't quickly give an official approval * have to decide whether comments are in a review or not
I'd like to comment on the commit and commit messages directly (eg, "Please rebase out the WIP comments here")
There's lots of friction when looking anywhere ELSE in the code during a code review. I'd like to be able to make a comment on eg, a line of code that needed to be changed in a file not in the review.
[+] [-] bpicolo|9 years ago|reply
One thing missing for me now is an interdiff feature. E.g. show me the diff minus <some initial commit that's a big vendor commit>.
@<username> is a pretty common pattern for mentioning people for reviews right now. Any chance we get a text shortcut for adding reviewers? Not strictly necessary but a nice-to-have
[+] [-] grrowl|9 years ago|reply
[+] [-] nojvek|9 years ago|reply
Being able to see reviewer status. Signed off, awaiting changes, reviewing, waiting for review.
[+] [-] tveita|9 years ago|reply
They don't even show up with an involves: filter
[+] [-] jtietema|9 years ago|reply
- keeping track of changes over the life time of a PR is impossible, you have review the whole thing over and over again - toggle notes (comments) on a per file basis is silly and unpractical on PR's with lots of files and comments. - there is no way to see which PR from a list you already approved.
And the list goes on and on. I have been using Github Enterprise at my new job since recently and it is by far the poorest of tools I have ever used to review code.
[+] [-] TempleOSV412|9 years ago|reply
[deleted]