Sheng’s comments are correct. GitHub will save review comments for ‘outdated’ commits, and they’re surfaced in the interface. I frequently amend then force push (i.e. change history) to address comments in order to keep my commits tidy. It’s always possible to expand a previous version selection to see the old comment history.
Example: https://github.com/awslabs/sockeye/pull/31 -Kellen From: Zha, Sheng Sent: Friday, December 22, 2017 12:34 AM To: dev@mxnet.incubator.apache.org Subject: Re: Apache MXNet Development Processes: Proposed update About comment on b), I think after rebase the existing review comments will remain in the PR even if rebase and history rewriting happens in the fork. The corresponding commit may be gone because of the change in commit history in the PR fork and thus the link change. Best regards, -sz On 12/15/17, 5:38 PM, "Markus Weimer" <mar...@weimo.de> wrote: On Fri, Dec 15, 2017 at 5:00 PM, Bhavin Thaker <bhavintha...@gmail.com> wrote: > a) It is NOT recommended for a committer to merge pull requests that the > committer authored. Instead the committer MUST get at least one approval > from another committer to merge his/her pull request. > +1 > - b) When you update a pull request with upstream, you MUST use rebase > to ensure that the pull request is easy to review by the community. See > the > how-to link here: > https://mxnet.incubator.apache.org/community/contribute.html Doesn't this potentially erase the review history on GitHub? Markus
