3 ms·

>The work has already been done. Try to capitalize on it, instead of dismissing it. Except it hasn't at all. This is the attitude of people who send pull reque

>The work has already been done. Try to capitalize on it, instead of dismissing it. Except it hasn't at all. This is the attitude of people who send pull requests to open-source projects and complain that they aren't accepted. The work is integrating that code with the rest of the system, ensuring it's compatible with future plans, ensuring that it's scalable and secure, and ensuring that the whole team can understand and maintain it. Successful projects don't just take any code that compiles and passes the tests, they are built in a thoughtful manner with high standards.

withinboredom 3y ago
> This is the attitude of people who send pull requests to open-source projects and complain that they aren't accepted. To be honest, I’m not sure how you got there from here… > The work is integrating that code with the rest of the system, ensuring it's compatible with future plans, ensuring that it's scalable and secure, and ensuring that the whole team can understand and maintain it. I actually suggested deprecating the library. I am not going to presume how hard that is. It might be changing 2 lines of code or 1000. No idea. That’s why we have brains though, we are able to figure these kinds of things out. As far as ensuring it’s compatible with future plans… do you honestly think a “random” maintainer of a library has your best interest at heart? That’s rather unlikely unless the maintainer happens to work for the company in question. In this particular case, it looks like someone volunteered to be that maintainer (probably due to naivety, but that’s beside the point) and they actually do have your future interests at heart. So to say that an employee doesn’t do work aligning with their company’s goals means you have bigger (political) problems than code review problems, hence why I said they should never have started working on it in the first place. For them to work on something like this for someone to review and professionally say “wtf” is a red flag, far and beyond code reviews; which is interestingly another reason why it is waaay too late to be having these conversations in a PR. It’s a smell that something in the team is fundamentally broken.
- Arainach 3y ago
  >To be honest, I’m not sure how you got there from here… "I've done all this work, get put of my way and submit" is the attitude in both cases. Congratulations, you wrote some code. It's not done until it meets team/project standards, and getting offended any time anyone has feedback makes you someone I'd never want to work with. The default answer to code review feedback is always "sure" unless you have a strong argument otherwise. It takes way less time to implement feedback than to pretend you're Socrates and question everything.
  - withinboredom 3y ago
    > "I've done all this work, get put of my way and submit" is the attitude in both cases. That’s quite a stretch. Nobody should be “in your way” to begin with. In a company, you are on a team and the point of a team is to work together; not get in each other’s way. That’s a sign of a dysfunctional team, which is how you end up getting team members working on the wrong thing or implementing things wrongly in the first place. Code not up to “standards” is not a reason to reject code, it’s a sign that something else is wrong on the team and that is the thing to be addressed, not the code. You aren’t going to resolve it in a code review. I don’t know how else to say that at this point. > The default answer to code review feedback is always "sure" unless you have a strong argument otherwise. I’ve definitely pointed out why a deleted line needs to be kept. I would not be surprised if that someone came back to tell me why it should be deleted. As a reviewer, I lack a lot of context even if I originally wrote the code. I’d never, ever, expect someone to blindly say “sure” on a code review. I don’t write perfect code and neither do my team members, so I don’t expect a perfect reviewer. A team works together to make the code perfect, nobody is writing mystery code and doing weird stuff without discussing it with the team first. And sure, people experiment and we review it, but with an architectural lens. Then when we are happy with the architecture, we actually implement it, together.

withinboredom 3y ago

> This is the attitude of people who send pull requests to open-source projects and complain that they aren't accepted. To be honest, I’m not sure how you got there from here… > The work is integrating that code with the rest of the system, ensuring it's compatible with future plans, ensuring that it's scalable and secure, and ensuring that the whole team can understand and maintain it. I actually suggested deprecating the library. I am not going to presume how hard that is. It might be changing 2 lines of code or 1000. No idea. That’s why we have brains though, we are able to figure these kinds of things out. As far as ensuring it’s compatible with future plans… do you honestly think a “random” maintainer of a library has your best interest at heart? That’s rather unlikely unless the maintainer happens to work for the company in question. In this particular case, it looks like someone volunteered to be that maintainer (probably due to naivety, but that’s beside the point) and they actually do have your future interests at heart. So to say that an employee doesn’t do work aligning with their company’s goals means you have bigger (political) problems than code review problems, hence why I said they should never have started working on it in the first place. For them to work on something like this for someone to review and professionally say “wtf” is a red flag, far and beyond code reviews; which is interestingly another reason why it is waaay too late to be having these conversations in a PR. It’s a smell that something in the team is fundamentally broken.

Arainach 3y ago
>To be honest, I’m not sure how you got there from here… "I've done all this work, get put of my way and submit" is the attitude in both cases. Congratulations, you wrote some code. It's not done until it meets team/project standards, and getting offended any time anyone has feedback makes you someone I'd never want to work with. The default answer to code review feedback is always "sure" unless you have a strong argument otherwise. It takes way less time to implement feedback than to pretend you're Socrates and question everything.
- withinboredom 3y ago
  > "I've done all this work, get put of my way and submit" is the attitude in both cases. That’s quite a stretch. Nobody should be “in your way” to begin with. In a company, you are on a team and the point of a team is to work together; not get in each other’s way. That’s a sign of a dysfunctional team, which is how you end up getting team members working on the wrong thing or implementing things wrongly in the first place. Code not up to “standards” is not a reason to reject code, it’s a sign that something else is wrong on the team and that is the thing to be addressed, not the code. You aren’t going to resolve it in a code review. I don’t know how else to say that at this point. > The default answer to code review feedback is always "sure" unless you have a strong argument otherwise. I’ve definitely pointed out why a deleted line needs to be kept. I would not be surprised if that someone came back to tell me why it should be deleted. As a reviewer, I lack a lot of context even if I originally wrote the code. I’d never, ever, expect someone to blindly say “sure” on a code review. I don’t write perfect code and neither do my team members, so I don’t expect a perfect reviewer. A team works together to make the code perfect, nobody is writing mystery code and doing weird stuff without discussing it with the team first. And sure, people experiment and we review it, but with an architectural lens. Then when we are happy with the architecture, we actually implement it, together.