5 ms·

Bonus if the vague comments relate to some refactoring that you'd like to do, the details of which exist only in your head. Meanwhile, yes, you know the existin

Bonus if the vague comments relate to some refactoring that you'd like to do, the details of which exist only in your head. Meanwhile, yes, you know the existing code is like X, and has been like that for years, but obviously I shouldn't have followed it and it's all wrong. For details on how I'd like it to be, please read your mind. Add an ellipsis to really drive home that only will any more explanations not be forthcoming but it should be evident that this is my problem...

asu_thomas 3y ago
> Bonus if the vague comments relate to some refactoring that you'd like to do I am extremely ashamed to say I have left a comment like this before. At the time, I was frustrated about never being able to discuss anything over a call. There is no excuse, though.
- bornfreddy 3y ago
  To be fair, it's not always a bad idea to present such ideas. However you need to make the comment resolvable, or better yet, create an issue for refactoring, add a small "fyi" comment and let them resolve it when they read it. This way they are aware of possible upcoming changes to this code but the MR/PR is not blocked.
  - bluefirebrand 3y ago
    I think "fyi" comments are valuable in PRs. Often all I want is for my team to approach similar problems differently in the future, not necessarily to refactor the immediate code. Or at the very least consider alternative approaches
    - desi_ninja 3y ago
      Also nit:
      - bluefirebrand 3y ago
        Yes, for sure. Although I much prefer to add automatic code formatting and linting and stuff to codebases to dramatically reduce the occurrence of 'nit:'
    - WorldMaker 3y ago
      Also such "fyi" comments can be a great way to get the refactoring discussion outside of just your own head. It can be a good start to boiling down the reasons for your refactor and getting external buy-in/validation that you are thinking down a useful path to the entire team and not just your own ego.
  - nielsole 3y ago
    I often add multiple comments and then approve the pr. My main question is always: does this make the codebase overall better or worse for some definition of good? If that bar is met, iterative improvements should not block deployment/merge. This way the value of the pr process is captured: preventing obviously bad changes from reaching prod and two-way knowledge sharing. Setting a higher bar for reviews more often than not blocks people for days without good reason.
    - bornfreddy 3y ago
      I always make sure that author can resolve my comments. If they can't do that, the pr is not approved - because I would like them to address something. If I approve it, then each of my comments should be resolvable.

asu_thomas 3y ago

> Bonus if the vague comments relate to some refactoring that you'd like to do I am extremely ashamed to say I have left a comment like this before. At the time, I was frustrated about never being able to discuss anything over a call. There is no excuse, though.

bornfreddy 3y ago
To be fair, it's not always a bad idea to present such ideas. However you need to make the comment resolvable, or better yet, create an issue for refactoring, add a small "fyi" comment and let them resolve it when they read it. This way they are aware of possible upcoming changes to this code but the MR/PR is not blocked.
- bluefirebrand 3y ago
  I think "fyi" comments are valuable in PRs. Often all I want is for my team to approach similar problems differently in the future, not necessarily to refactor the immediate code. Or at the very least consider alternative approaches
  - desi_ninja 3y ago
    Also nit:
    - bluefirebrand 3y ago
      Yes, for sure. Although I much prefer to add automatic code formatting and linting and stuff to codebases to dramatically reduce the occurrence of 'nit:'
  - WorldMaker 3y ago
    Also such "fyi" comments can be a great way to get the refactoring discussion outside of just your own head. It can be a good start to boiling down the reasons for your refactor and getting external buy-in/validation that you are thinking down a useful path to the entire team and not just your own ego.
- nielsole 3y ago
  I often add multiple comments and then approve the pr. My main question is always: does this make the codebase overall better or worse for some definition of good? If that bar is met, iterative improvements should not block deployment/merge. This way the value of the pr process is captured: preventing obviously bad changes from reaching prod and two-way knowledge sharing. Setting a higher bar for reviews more often than not blocks people for days without good reason.
  - bornfreddy 3y ago
    I always make sure that author can resolve my comments. If they can't do that, the pr is not approved - because I would like them to address something. If I approve it, then each of my comments should be resolvable.