[Github-comments] [geany/geany] build.c: Remove g_ptr_array_foreach with gpointer user_data & update HACKING (#2270)

[Nick Treleaven]

> Edit: Alternatively, we could say that if a PR has been submitted for over N days and has no Github "requested changes" on it, it's safe to merge.

Sounds good, but for simple changes N should be low. Sometimes there's a pull that causes merge conflicts on other pulls (not the case for this one though),  then it's pragmatic to merge quite soon. Otherwise pulls are harder to review because they include dependent commits too.

It's good to usually merge stuff without waiting too long, particularly when follow-up work is likely. In master those changes will likely get tested more as more people will use them. If there's a problem, we can revert. I think this project has been too hesitant in the years I was away to merge code, work from developers can end up sitting in the review queue indefinitely. Maybe none of us will ever get around to reviewing it, despite the changes being very useful and working. In those cases, we should merge if the code is well written and the changes are wanted even if we won't test them ourselves, assuming the author says they've tested them adequately.

> IOW, if you have an issue with a PR but no time to do a proper review, do a "requested changes" review with a short comment explaining as much.

I think a comment 'please wait for my review' should be enough, unless there are lots of other comments also.

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/geany/geany/pull/2270#issuecomment-526670021
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.geany.org/pipermail/github-comments/attachments/20190830/cf8637be/attachment.html>