4 ms·

um... what do you mean "not having vetoes in the first place"? Author opens a PR. Reviewer starts reviewing. The pull request merging should be blocked until r

by theamk 4y ago

um... what do you mean "not having vetoes in the first place"? Author opens a PR. Reviewer starts reviewing. The pull request merging should be blocked until review is done, if anything to prevent mistakes. As review progresses, there is a comment which author refuses to fix. Since all parties must mark review as "complete", the PR is blocked cannot be merged until the comment can be resolved. Author and reviewer go to manager, which decides on one way or another. The manager's resolution is applied -- either reviewer resolves the comment, or author rewrites PR to fix the defect. Was there any "veto" applied? It is not like there is a button labeled "veto" during review process, it is just that some comments take longer to resolve some others. Or is your ideal system when reviews are non-binding and author can press "merge" button at any time despite having open review comments? In this case it is pretty horrible for reviewers - code review is a very hard job and I would be pretty disheartened if the author just walks away instead of resolving discussions. I think if your review is like that, you might as well just code reviews altogether as it is a total waste of time.

lapcat 4y ago
> Author opens a PR. Going back to what I said in an earlier comment: "This seems to be just more evidence that for some reason, some people can't seem to imagine code review without pull requests. Maybe you have to be older, and worked before git and GitHub existed? I don't know. Let me spell it out more clearly: a pull request is when the reviewer can reject your commit and force you to meet the reviewer's demands. Whereas you can have mandatory code review without pull requests, where the mandate is just that someone has to examine your commit and comment on it." So let's start over: 1) Author commits to a branch. 2) Someone else reviews that commit and gives feedback. 3) If necessary, as determined by author based on the feedback, author modifies code. This is an async process. There's no blocking.
- theamk 4y ago
  Ah, I worked in a place like this. We had a lightweight Fagan inspection process (with 4 roles and all that stuff) and some scripts we wrote to ensure each commit was either reviewed or explicitly skipped. It was OK - definitely better than nothing, but with its own problems. Biggest defects were the worst - lets say someone made a new library function and inspection found out that function should not exist at all. By the time it's time to make a fix, code might already be used. The latency was not so great -- to save time, we often requested a review of entire patch series so the time from the first commit to the first comment was pretty long. Later on for business reasons we had to stop working on that codebase and start all new code from scratch. Everyone on the team voted to switch to "pull review + code review before commit" and we never looked back. Thinking back about this, the main reason to use post-inspection was that the tooling sucked. Back then we had SVN and branches were super heavy, the easiest way to share code was to email files around, and combining yours and someone else's unpublished changed was arcane process which could easily lead to data loss. Additionally, all sort of tooling, like advanced tests and staging environment required code on master. So making merge quick and easy was the only solution. Luckily by the time we started again git was around (but not github, and we self-hosted everything anyway), so we improved the tooling itself. Turns out having lightweight branches is great! And being able to tell a colleague "PTAL at my branch" is also great! And we improved tooling so we could push any branch to staging or run extended tests on any commit! And we can review the code and fix it before having entire company use it! So the fact that a PR might take a day or two to land was not a big deal at all. If you are still on the post-commit reviews, take a look at your processes. The new features are great, and you might be missing out on them. (This assumes a medium sized team. If it's just 2 people working on a codebase, the benefits are much less and costs are relatively bigger)

by theamk 4y ago

lapcat 4y ago

> Author opens a PR. Going back to what I said in an earlier comment: "This seems to be just more evidence that for some reason, some people can't seem to imagine code review without pull requests. Maybe you have to be older, and worked before git and GitHub existed? I don't know. Let me spell it out more clearly: a pull request is when the reviewer can reject your commit and force you to meet the reviewer's demands. Whereas you can have mandatory code review without pull requests, where the mandate is just that someone has to examine your commit and comment on it." So let's start over: 1) Author commits to a branch. 2) Someone else reviews that commit and gives feedback. 3) If necessary, as determined by author based on the feedback, author modifies code. This is an async process. There's no blocking.

theamk 4y ago
Ah, I worked in a place like this. We had a lightweight Fagan inspection process (with 4 roles and all that stuff) and some scripts we wrote to ensure each commit was either reviewed or explicitly skipped. It was OK - definitely better than nothing, but with its own problems. Biggest defects were the worst - lets say someone made a new library function and inspection found out that function should not exist at all. By the time it's time to make a fix, code might already be used. The latency was not so great -- to save time, we often requested a review of entire patch series so the time from the first commit to the first comment was pretty long. Later on for business reasons we had to stop working on that codebase and start all new code from scratch. Everyone on the team voted to switch to "pull review + code review before commit" and we never looked back. Thinking back about this, the main reason to use post-inspection was that the tooling sucked. Back then we had SVN and branches were super heavy, the easiest way to share code was to email files around, and combining yours and someone else's unpublished changed was arcane process which could easily lead to data loss. Additionally, all sort of tooling, like advanced tests and staging environment required code on master. So making merge quick and easy was the only solution. Luckily by the time we started again git was around (but not github, and we self-hosted everything anyway), so we improved the tooling itself. Turns out having lightweight branches is great! And being able to tell a colleague "PTAL at my branch" is also great! And we improved tooling so we could push any branch to staging or run extended tests on any commit! And we can review the code and fix it before having entire company use it! So the fact that a PR might take a day or two to land was not a big deal at all. If you are still on the post-commit reviews, take a look at your processes. The new features are great, and you might be missing out on them. (This assumes a medium sized team. If it's just 2 people working on a codebase, the benefits are much less and costs are relatively bigger)