Hacker Newsnew | past | comments | ask | show | jobs | submitlogin

How do you phrase these warnings? "Next time.."? I have a hard time being serious with my own warnings if it's fine enough for now.


We prefix our comments with "minor:", and all employees know that this means it's something that would be nice but not necessary to merge


Use the delete key. (My apologies for the snark, but I'm kinda serious, don't send those type of review comments at all).

If it's fine for now, it's fine. If it's not fine - then speak up.

If you do have such concerns, "fine for now" - that is something for an offline conversation - outside of the context of a CR. Which is also a good litmus test for how important a comment is. Would you schedule a 15 minute meeting to discuss the issue with your colleague? If not, then why is it okay to do that to someone during a CR?


"This is okay for now, but we should think about how we want to serialize these objects. Feel free to remove the N^2 algorithm in a follow up."


That works great in a setting where you are both employees of the same company, and you respect each other, but it often doesn't work in the open source world, people just disappear and you never hear from them again. It is possible that they do file follow-ups, but in my experience it's rare.


Yes, even within a company my threshold for accepting a change can vary pretty widely depending on my experience and relationship with the author. For an external contributor or someone I've never collaborated with (by reviewing code or having my code reviewed), I don't accept the code until almost everything is worked out to my satisfaction. With someone I work with regularly, it's not uncommon to accept a change with a comment like "this is all good, but you need to take X into account which will change almost everything in this patch" (I exaggerate, but only slightly). I know whether an update could be problematic and whether it is necessary to see it again. Sometimes there are a couple of obvious ways that something could be done, they picked one but weren't tied to it if I had a reason for picking a different one, I picked a different one for $REASON.

Most are somewhere in between.

Though in some ways it works the other way around. For an unfamiliar open source contributor, I need to be confident that the change is worthwhile. I will be lenient on stylistic things, and I'll just land their patch and then fix it up afterwards. For someone I've worked with a bunch (whether a familiar contributor or a coworker), I will trust their opinion on the underlying quality of a change, but be less tolerant of unnecessary stylistic differences since they should have already come into alignment on those and it's more likely to be an oversight if they missed something. (Plus, I don't want to be fixing up their changes after the fact, given that >90% of patches will come from regular contributors.)


To add to this, "no time like now" is real. Follow-ups rarely happen, tracking them is a pain. Making someone accountable for it is unfair, particularly if they disagree with the follow up (and presumably they do, otherwise they would have done the thing in the first place).




Guidelines | FAQ | Lists | API | Security | Legal | Apply to YC | Contact

Search: