3 ms·

Im still unclear how people get bit by this kind of thing. The way ive always operated is that nothing in a Code Review can stop a merge unless its a rule writ

Im still unclear how people get bit by this kind of thing. The way ive always operated is that nothing in a Code Review can stop a merge unless its a rule written down before the review or its just a blatantly obvious incorrect miss of requirements or functionality. I love seeing peoples comments about how things can be improved in reviews and learning new approaches or context, but if there's not reference to a written rule I should have known about beforehand anyway, I'm not changing it if I don't agree with it.

tyleo 3y ago
I disagree with this. For example, lets say an engineer implements a 1000 line function and another engineer says, 'A similar function already exists. Just add a parameter and change these 2 lines'. I don't think we need to over index on written rules for common sense recommendations like this. I'd be happy to see the first engineer blocking another's PR on the request. In the long run it isn't viable to optimize for shipping, I think a lot of the optimization has to be directed towards maintenance.
- arcbyte 3y ago
  Common sense is never common. I'm sure you can find something written to block nonsense like that - is it fully unit tested? You do have something written that requires unit tests right? But at the end of the day, if you're on a team with somebody like that then you NEED written rules to deal with that nonsense and the Code Review process is the first step to identify it. The nonsense gets merged the first time, but not before you add a written rule against it going forward. Problem solved.
ravenstine 3y ago
> The way ive always operated is that nothing in a Code Review can stop a merge unless its a rule written down before the review or its just a blatantly obvious incorrect miss of requirements or functionality. I've never worked on a team where code reviews worked that way for anyone besides maybe the team lead. Even if nobody uses the GitHub feature to add a blocking review comment, there's always this expectation that nearly every review comment has to be treated as "right" by default, even if it's ultimately based on opinion, and your job as the submitter is to either debate or accept every opinion before merging; this of course means a few changes lines can take days to get merged, unless everyone jumps on a call to have a synchronous code review, which nobody wants to do every single day. The reality is most organizations don't write anything down, and even when they occasionally do, there's a high probability that said documentation is outdated. So while I like the idea of the written rule principle, I don't see it being widely adopted, and there are many more developers who actually believe it's better that every decision just lives in people's heads.
- arcbyte 3y ago
  If you have need unanimous consent to merge then yea you'd probably need to address every single comment. Otherwise you can always get the one or two approvals you need from people you know are smart. If they're good with it and nobody is pointing out rule violations, just nonsense, I've never felt at any fortune 500 company, that I couldnt merge. I do see people who seem to believe that, but they're always junior and it's all in their heads. Nobody ever said that you need to listen to every single comment. Write good code, incorporate good suggestions, ship stuff fast, test well, deliver working software that solves customer problems. That's your job.

tyleo 3y ago

I disagree with this. For example, lets say an engineer implements a 1000 line function and another engineer says, 'A similar function already exists. Just add a parameter and change these 2 lines'. I don't think we need to over index on written rules for common sense recommendations like this. I'd be happy to see the first engineer blocking another's PR on the request. In the long run it isn't viable to optimize for shipping, I think a lot of the optimization has to be directed towards maintenance.

arcbyte 3y ago
Common sense is never common. I'm sure you can find something written to block nonsense like that - is it fully unit tested? You do have something written that requires unit tests right? But at the end of the day, if you're on a team with somebody like that then you NEED written rules to deal with that nonsense and the Code Review process is the first step to identify it. The nonsense gets merged the first time, but not before you add a written rule against it going forward. Problem solved.

ravenstine 3y ago

> The way ive always operated is that nothing in a Code Review can stop a merge unless its a rule written down before the review or its just a blatantly obvious incorrect miss of requirements or functionality. I've never worked on a team where code reviews worked that way for anyone besides maybe the team lead. Even if nobody uses the GitHub feature to add a blocking review comment, there's always this expectation that nearly every review comment has to be treated as "right" by default, even if it's ultimately based on opinion, and your job as the submitter is to either debate or accept every opinion before merging; this of course means a few changes lines can take days to get merged, unless everyone jumps on a call to have a synchronous code review, which nobody wants to do every single day. The reality is most organizations don't write anything down, and even when they occasionally do, there's a high probability that said documentation is outdated. So while I like the idea of the written rule principle, I don't see it being widely adopted, and there are many more developers who actually believe it's better that every decision just lives in people's heads.

arcbyte 3y ago
If you have need unanimous consent to merge then yea you'd probably need to address every single comment. Otherwise you can always get the one or two approvals you need from people you know are smart. If they're good with it and nobody is pointing out rule violations, just nonsense, I've never felt at any fortune 500 company, that I couldnt merge. I do see people who seem to believe that, but they're always junior and it's all in their heads. Nobody ever said that you need to listen to every single comment. Write good code, incorporate good suggestions, ship stuff fast, test well, deliver working software that solves customer problems. That's your job.