4 ms·

The core issue here is reviewer saying “if you change this, you also have to fix other pending issues in codebase”. That should be the point of push back “Grea

by nojvek 3y ago

The core issue here is reviewer saying “if you change this, you also have to fix other pending issues in codebase”. That should be the point of push back “Great feedback, and appreciate the direction to improve code quality, however the implications of changing Y means we need X, Y, Z approval and it will many days. I am making a tech debt task with what you described and will be done in a follow up PR based on priority and bandwidth. Let’s focus on what it would take to get this surgical PR out.” The biggest lesson I learned is to make focused PRs, and learn to pushback when reviewer asks for a scope creep. Mostly other engineers have been pragmatic. It’s unrelated to number of lines as you can reformat while codebase but not change anything logically, or change some feature flags which could have a huge impact. But ONE FOCUSED CHANGE AT A TIME

mabbo 3y ago
An even better solution, imho, is that rules should be automated. New rules are added, and the automation adds rule override comments to every existing violation - which also lets you track them. When you need to rush code out that needs to violate a rule, you add an override comment as well, with your name assigned to fix it. Then over time, you build a culture that wants to fix these rule violations separate from feature development.
hinkley 3y ago
Score creepers have no idea the architectural damage they cause. When you get over invested in one block of code, people will try to route around it. Then you get layers of these and pretty soon your code is the moral equivalent of Atlanta, GA, which is notorious for the number of ring roads it has.
foogazi 3y ago
> The biggest lesson I learned is to make focused PRs, and learn to pushback when reviewer asks for a scope creep. Whenever that happens just add a TODO ticket. Unblock production and the system is not worse for it
figassis 3y ago
Usually I avoid making any improvement that is not directly related to the tasks at hand. Even adding a missing semicolon risks bringing this to the attention of an overzealous reviewer that will then make me follow a rabbit hole of legacy fixes. I’d rather create issues silently so I don’t forget instead of even adding a FIXME or TODO. I think this part of reviews is broken. Resolving tech debt should not be a requirement for completing a task, it should be planned.
- Tainnor 3y ago
  I fundamentally disagree. If every minor fix needs to be planned, it never gets done. The boyscout rule wins, and if your reviewers are not pragmatic, then that's an issue with their attitude.
  - Rexxar 3y ago
    Just do the small fixes in a different commit and do a multi-commit PR.
- tremon 3y ago
  Resolving tech debt should not be a requirement for completing a task, it should be planned. Such tasks will never be planned because there's always higher-priority tasks to be done. I actively encourage my developers to clean up the code they'll be working on, otherwise you're just piling tech debt upon existing tech debt. The only exception is repository-wide refactorings: if those are necessary to implement a feature, we block the feature and create a prerequisite task outlining the refactoring needed. But this is mostly to aid the review process, in my mind these two tasks are still one (and if I work on it myself, I usually already have the feature half-implemented so I can double-check that the refactoring does indeed make the feature easier to implement).
  - figassis 3y ago
    I agree this is ideal, but my point is, there were instances where you try to cleanup, and you end up pulling an infinite thread because the feature you're working on is stacked on top of 10 other features, each of which were duct taped together, and product forgot about this, and they think if it's working it must be very straightforward to just add this one thing. Your choice is to duct tape again or to refactor the 10 features. So you dare change one like of code that optimizes one of the previous features and the reviewer asks you to also optimize A, encapsulate B and replace method C with new and improved method D that does some of what you need but you now need to rework your solution to conform to that. So I often leave that semicolon alone.
Paul-Craft 3y ago
> The core issue here is reviewer saying “if you change this, you also have to fix other pending issues in codebase”. I don't agree. IMO, the worst issue being displayed here is that of the "6 days to change 1 line of code," almost half of them elapsed before an engineer ever looked at the issue. If this is such a high priority issue that the company would "need" (scare quotes intentional) to do a layoff if it's not implemented soon, those 2-3 days before anyone looked at it should never have happened. And that is apparently what passes for "the fast track" in this development process. And then there's the 2 days at the end of this 6 day period where it looks like nothing happened, because the test plan was deemed insufficient. "If you change this, you also have to fix other pending issues" only took up 2 hours here. There are at least 2 or 3 other things I could fill in here that I'd flag as core issues with this process before I'd even think about dealing with this part of the process.
- nojvek 3y ago
  You are correct. Orthogonal to the PR process is incompetence of management being unable to identify key stakeholders and communicating the urgency and importance. It should have been some VP realizes how important this is. Gets the security folks, testing folks, Senior devs, team manager in a room and task them at figuring how how to solve this very specific problem before it impacts a layoff. I didn’t really understand the difference between good and bad management until I had multiple different managers and VPs over a decade at different companies. The difference between good and great is night and day once you witness an exemplary communicator who can get the right people together and give them focus.
rad_gruchalski 3y ago
“I am very sorry to hear but my job depends on making sure we follow our procedures. Please follow outlined procedures before your change is promoted to production. Thank you for your cooperation. Sincerely, your IT department.”
guluarte 3y ago
it is not about code quality, it is about power lmao

by nojvek 3y ago

mabbo 3y ago

An even better solution, imho, is that rules should be automated. New rules are added, and the automation adds rule override comments to every existing violation - which also lets you track them. When you need to rush code out that needs to violate a rule, you add an override comment as well, with your name assigned to fix it. Then over time, you build a culture that wants to fix these rule violations separate from feature development.

hinkley 3y ago

Score creepers have no idea the architectural damage they cause. When you get over invested in one block of code, people will try to route around it. Then you get layers of these and pretty soon your code is the moral equivalent of Atlanta, GA, which is notorious for the number of ring roads it has.

foogazi 3y ago

> The biggest lesson I learned is to make focused PRs, and learn to pushback when reviewer asks for a scope creep. Whenever that happens just add a TODO ticket. Unblock production and the system is not worse for it

figassis 3y ago

Usually I avoid making any improvement that is not directly related to the tasks at hand. Even adding a missing semicolon risks bringing this to the attention of an overzealous reviewer that will then make me follow a rabbit hole of legacy fixes. I’d rather create issues silently so I don’t forget instead of even adding a FIXME or TODO. I think this part of reviews is broken. Resolving tech debt should not be a requirement for completing a task, it should be planned.

Tainnor 3y ago
I fundamentally disagree. If every minor fix needs to be planned, it never gets done. The boyscout rule wins, and if your reviewers are not pragmatic, then that's an issue with their attitude.
- Rexxar 3y ago
  Just do the small fixes in a different commit and do a multi-commit PR.
tremon 3y ago
Resolving tech debt should not be a requirement for completing a task, it should be planned. Such tasks will never be planned because there's always higher-priority tasks to be done. I actively encourage my developers to clean up the code they'll be working on, otherwise you're just piling tech debt upon existing tech debt. The only exception is repository-wide refactorings: if those are necessary to implement a feature, we block the feature and create a prerequisite task outlining the refactoring needed. But this is mostly to aid the review process, in my mind these two tasks are still one (and if I work on it myself, I usually already have the feature half-implemented so I can double-check that the refactoring does indeed make the feature easier to implement).
- figassis 3y ago
  I agree this is ideal, but my point is, there were instances where you try to cleanup, and you end up pulling an infinite thread because the feature you're working on is stacked on top of 10 other features, each of which were duct taped together, and product forgot about this, and they think if it's working it must be very straightforward to just add this one thing. Your choice is to duct tape again or to refactor the 10 features. So you dare change one like of code that optimizes one of the previous features and the reviewer asks you to also optimize A, encapsulate B and replace method C with new and improved method D that does some of what you need but you now need to rework your solution to conform to that. So I often leave that semicolon alone.

Paul-Craft 3y ago

> The core issue here is reviewer saying “if you change this, you also have to fix other pending issues in codebase”. I don't agree. IMO, the worst issue being displayed here is that of the "6 days to change 1 line of code," almost half of them elapsed before an engineer ever looked at the issue. If this is such a high priority issue that the company would "need" (scare quotes intentional) to do a layoff if it's not implemented soon, those 2-3 days before anyone looked at it should never have happened. And that is apparently what passes for "the fast track" in this development process. And then there's the 2 days at the end of this 6 day period where it looks like nothing happened, because the test plan was deemed insufficient. "If you change this, you also have to fix other pending issues" only took up 2 hours here. There are at least 2 or 3 other things I could fill in here that I'd flag as core issues with this process before I'd even think about dealing with this part of the process.

nojvek 3y ago
You are correct. Orthogonal to the PR process is incompetence of management being unable to identify key stakeholders and communicating the urgency and importance. It should have been some VP realizes how important this is. Gets the security folks, testing folks, Senior devs, team manager in a room and task them at figuring how how to solve this very specific problem before it impacts a layoff. I didn’t really understand the difference between good and bad management until I had multiple different managers and VPs over a decade at different companies. The difference between good and great is night and day once you witness an exemplary communicator who can get the right people together and give them focus.

rad_gruchalski 3y ago

“I am very sorry to hear but my job depends on making sure we follow our procedures. Please follow outlined procedures before your change is promoted to production. Thank you for your cooperation. Sincerely, your IT department.”

guluarte 3y ago

it is not about code quality, it is about power lmao