Hacker News Reader

Comment by bornfreddy | Hacker News Reader

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.

View on HN

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

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

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