4 ms·

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 o

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.
- lelanthran 3y ago
  > 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. While I broadly agree on the boy-scout rule, it can severely throw out (already poor) estimates. It shouldn't be a rule ("always clean up the shit you see when working on something else"), it should be optional ("If it doesn't mess up your original goal, clean up as much as is possible within the estimated time").

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.
lelanthran 3y ago
> 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. While I broadly agree on the boy-scout rule, it can severely throw out (already poor) estimates. It shouldn't be a rule ("always clean up the shit you see when working on something else"), it should be optional ("If it doesn't mess up your original goal, clean up as much as is possible within the estimated time").