4 ms·

this isn't about pull requests at all. this is about gating, done via sending patches via email, or branch pulling, or some other mechanism. You seem to be aga

this isn't about pull requests at all. this is about gating, done via sending patches via email, or branch pulling, or some other mechanism. You seem to be against the concept of someone having the ability to veto your submission, and that is indeed what most people think of the purpose of code review is. If it doesn't pass muster, it doesn't get merged into the main branch.

lapcat 4y ago
> this isn't about pull requests at all. this is about gating I know. My original comment said, "The author keeps equating code review with pull requests and gated check-ins." > You seem to be against the concept of someone having the ability to veto your submission Correct. > that is indeed what most people think of the purpose of code review is. I don't know about "most" people, but the people who think that are wrong. :-) > If it doesn't pass muster According to whom, is the question.
- theamk 4y ago
  Answer is simple: according to people who would be suffering from bad code. I've had seen code reviews done by people who are not working on the code in question and never will, and this is dysfunctional and should never happen. If that's your case then I am sorry and yes, make code-review optional, non-blocking or even better cancel them all together. But the proper code review is done by your peers working on the same part of code. When I put a blocking comment saying "this function called GetFoo now deletes database, submits payment and returns Bar, please rename it to match its function" this is not done because I read too much books and have weird ideas... This comment was written because I know that next week I will be adding new features and debugging production failures and having bad code will actually impede my progress. Of course people may disagree what "good" is, so there should be some way to resolve conflicts. I've seen teams with very strong technical leader who had the final word. Or the one place which had "review meetings" for author/reviewer disagreements which anyone could attend and which would vote on resolutions. Either way, make sure you have a wiki page or something where you can write down what you decide on (for example: "no review defect should complain about punctuation in comments"), so that the same questions don't come up again and again. (Btw, if you have problems with other people reviewing your code, this is where microservices shine. You have your own repo and service, they have their own repo and service. No one touches other's code and thus has no grounds to impose code review. Just make sure to have good monitoring solution so that if main app falls down you know who's service to blame.)
  - lapcat 4y ago
    > Answer is simple: according to people who would be suffering from bad code. The users? > But the proper code review is done by your peers working on the same part of code. Of course. However, that's tangential to the issue of code vetoes. > Of course people may disagree what "good" is, so there should be some way to resolve conflicts. I've seen teams with very strong technical leader who had the final word. That's fine, but many conflicts can be avoided by not having vetoes in the first place. If the coder refuses to fix code that the reviewer considers not shippable, then they can go to the manager to resolve the conflict. The manager always has a veto, but otherwise there's no reason for coworkers to have a veto over each other. > if you have problems with other people reviewing your code I don't.
    - 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.

lapcat 4y ago

> this isn't about pull requests at all. this is about gating I know. My original comment said, "The author keeps equating code review with pull requests and gated check-ins." > You seem to be against the concept of someone having the ability to veto your submission Correct. > that is indeed what most people think of the purpose of code review is. I don't know about "most" people, but the people who think that are wrong. :-) > If it doesn't pass muster According to whom, is the question.

theamk 4y ago
Answer is simple: according to people who would be suffering from bad code. I've had seen code reviews done by people who are not working on the code in question and never will, and this is dysfunctional and should never happen. If that's your case then I am sorry and yes, make code-review optional, non-blocking or even better cancel them all together. But the proper code review is done by your peers working on the same part of code. When I put a blocking comment saying "this function called GetFoo now deletes database, submits payment and returns Bar, please rename it to match its function" this is not done because I read too much books and have weird ideas... This comment was written because I know that next week I will be adding new features and debugging production failures and having bad code will actually impede my progress. Of course people may disagree what "good" is, so there should be some way to resolve conflicts. I've seen teams with very strong technical leader who had the final word. Or the one place which had "review meetings" for author/reviewer disagreements which anyone could attend and which would vote on resolutions. Either way, make sure you have a wiki page or something where you can write down what you decide on (for example: "no review defect should complain about punctuation in comments"), so that the same questions don't come up again and again. (Btw, if you have problems with other people reviewing your code, this is where microservices shine. You have your own repo and service, they have their own repo and service. No one touches other's code and thus has no grounds to impose code review. Just make sure to have good monitoring solution so that if main app falls down you know who's service to blame.)
- lapcat 4y ago
  > Answer is simple: according to people who would be suffering from bad code. The users? > But the proper code review is done by your peers working on the same part of code. Of course. However, that's tangential to the issue of code vetoes. > Of course people may disagree what "good" is, so there should be some way to resolve conflicts. I've seen teams with very strong technical leader who had the final word. That's fine, but many conflicts can be avoided by not having vetoes in the first place. If the coder refuses to fix code that the reviewer considers not shippable, then they can go to the manager to resolve the conflict. The manager always has a veto, but otherwise there's no reason for coworkers to have a veto over each other. > if you have problems with other people reviewing your code I don't.
  - 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.