5 ms·

I absolutely trust my co-workers to not be overly pedantic and opinionated. If they were, I would raise it with my line manager and try to get the issue fed ba

by eterm 4y ago

I absolutely trust my co-workers to not be overly pedantic and opinionated. If they were, I would raise it with my line manager and try to get the issue fed back to them. What kind of workplace do you not trust your coleagues to be reasonable? I don't trust myself nor anyone to not make mistakes or have blind-spots that pull requests and code review helps catch. Here's an example from today of an issue caught code review, that could have been a bug that easily slipped through testing only to become a hard debug issue months or years later: "You're assuming the order here, but it should be set explicitly". Caught by the reviewer, and easily fixed in seconds. But that could have been a really expensive bug to fix later. And it wasn't really through a lack of competence, just an oversight because the data wasn't necessarily sorted at that point. And the gating helps enforce and audit that code is reviewed, that everyone can go back and read the reviews to understand the journey the code went through. If there's later a bug in that area, there are now two different people I can know to ask first about that code. Making code review optional is a good way to get it waved through with little oversight. Having the people who okay the request share responsibility for fixing that code is a good way to motivate better code reviews as well as share knowledge. Why would a coworker want to block your progress? Maybe I've never worked anywhere toxic enough to understand.

lapcat 4y ago
> Making code review optional is a good way to get it waved through with little oversight. Me: "Code review is fine and necessary." How did you turn "necessary" into "optional"? 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. > Why would a coworker want to block your progress? Maybe I've never worked anywhere toxic enough to understand. Have you... heard of large corporations? ;-)
- johnny22 4y ago
  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.)
peterangular 4y ago
> What kind of workplace do you not trust your colleagues to be reasonable? This reads a bit weird so I'm interpreting it as "what kind of workplace causes you to not trust your colleagues to be reasonable?" Lots. There are many places where developers will play outright power games in order to posture and preserve their position. The PR is a big spot where this can shine through in black/white. As a manager, I really have to police GitHub/GitLab/Bitbucket to ensure that this isn't happening. I've had juniors, equals, and seniors hold up months worth of work for arbitrary reasons, purposefully pushing past deadlines. Some even admit to it behind closed doors/in meetings. Toxic work places surely exist. --- To be clear, I am 100% in support of peer reviewed work as a SWE (PR's, external auditing, etc)... I'm just expressing that I've experienced the check/balances of peer reviewed work weaponized, typically to feign importance or as an unfair attempt to advance someone's career. I feel like it's just the human condition that some people will just take advantage.
- justinclift 4y ago
  > There are many places where developers will play outright power games in order to posture and preserve their position. Isn't that a strong signal that the culture is bad? Maybe not of the entire company, but at least of the team that's happening in.
  - peterangular 4y ago
    > Isn't that a strong signal that the culture is bad? It can be - but it is also isolated to pockets/individuals within a company which doesn't necessarily define the culture. > Maybe not of the entire company Absolutely agree, and that's not well captured in my comment on second read. It can be at the company level, at the team level, and all the way down to the individual level - obviously in any configuration/mix. Sometimes it's not on the company that they had a hypothetical bad hire as long as they take corrective action, just to give an example.

by eterm 4y ago

lapcat 4y ago

> Making code review optional is a good way to get it waved through with little oversight. Me: "Code review is fine and necessary." How did you turn "necessary" into "optional"? 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. > Why would a coworker want to block your progress? Maybe I've never worked anywhere toxic enough to understand. Have you... heard of large corporations? ;-)

johnny22 4y ago
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.)

peterangular 4y ago

> What kind of workplace do you not trust your colleagues to be reasonable? This reads a bit weird so I'm interpreting it as "what kind of workplace causes you to not trust your colleagues to be reasonable?" Lots. There are many places where developers will play outright power games in order to posture and preserve their position. The PR is a big spot where this can shine through in black/white. As a manager, I really have to police GitHub/GitLab/Bitbucket to ensure that this isn't happening. I've had juniors, equals, and seniors hold up months worth of work for arbitrary reasons, purposefully pushing past deadlines. Some even admit to it behind closed doors/in meetings. Toxic work places surely exist. --- To be clear, I am 100% in support of peer reviewed work as a SWE (PR's, external auditing, etc)... I'm just expressing that I've experienced the check/balances of peer reviewed work weaponized, typically to feign importance or as an unfair attempt to advance someone's career. I feel like it's just the human condition that some people will just take advantage.

justinclift 4y ago
> There are many places where developers will play outright power games in order to posture and preserve their position. Isn't that a strong signal that the culture is bad? Maybe not of the entire company, but at least of the team that's happening in.
- peterangular 4y ago
  > Isn't that a strong signal that the culture is bad? It can be - but it is also isolated to pockets/individuals within a company which doesn't necessarily define the culture. > Maybe not of the entire company Absolutely agree, and that's not well captured in my comment on second read. It can be at the company level, at the team level, and all the way down to the individual level - obviously in any configuration/mix. Sometimes it's not on the company that they had a hypothetical bad hire as long as they take corrective action, just to give an example.