I should have said that the current policy isn’t absolute. I agree that a squash/rebase can be beneficial if there are many commits. It’s a trade off, and the reviewer should make the call, not the author of the PR.
Julian > On Sep 6, 2023, at 8:45 AM, Julian Hyde <[email protected]> wrote: > > Personally, I don’t have a problem reviewing PRs that have many commits. The > GitHub web UI and command-like tools like *git diff main” make it easy to see > the whole change. > > Conversely, if I have reviewed a PR and requested changes, I would much > rather that the author makes those changes in an additional commit or > commits. I can easily see what they did to address my concerns, and I don’t > need to re-read the changes I already reviewed. > > Furthermore, if an earlier review had comments against specific lines, GitHub > has trouble keeping those comments in place when there is a force-push. > > So, I ask authors to not force-push or rebase unless a reviewer explicitly > asks them to. I have heard other committers make similar requests, so I would > say this is the current consensus policy in Calcite. (Of course, > reconsidering that policy, as we are doing in this discussion, is just fine.) > > Julian > >> On Sep 6, 2023, at 8:05 AM, LakeShen <[email protected]> wrote: >> >> Hi devs, >> >> I found that each PR has a lot of commits and file changed, which could >> make it difficult to review. One approach is for the author of the PR to >> rebase the latest code in the main branch and force push again. Best, >> LakeShen
