Hacker News Reader

Comment by adhesive_wombat | Hacker News Reader

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...

View on HN

> 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.

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.

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

Also nit:

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:'

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.

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.

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.

Reply on news.ycombinator.com