4 ms·
> 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 tim
by 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 agoTo 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 agoI 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 agoAlso nit:
- bluefirebrand 3y agoYes, 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 agoAlso 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 agoI 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 agoI 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.