Hello everyone, I share Julian's opinion about this, and I have seen other reviewers making the same request (hold off from squashing while the review is in progress), but never the opposite (please squash as there are many commits), but my experience might just be anecdotal.
When I author a PR (not only here), I try to separate my contribution into small and purposeful commits, so that people can clearly skip eventual refactoring commits, clearly see code modifications and tests changes/additions. I also sometimes find the commit messages coming from the code changes to become the extended commit message for my squashed commit, for some non-trivial changes. IMO it's a way to guide the reviewer through your PR, more informative than a single commit with all changes at the same time. As a reviewer, I like when a PR is crafted this way, as it allows easily to split the review process into different time frames, following the commits. Of course sometimes I still need to go back and forth between them (like checking the code change when looking at the tests), but it's better than just going file by file and marking them as "reviewed" in the GitHub UI. As Julian says, there are different styles and it's always a trade-off between multiple factors. Best regards, Alessandro On Wed, 6 Sept 2023 at 17:50, Julian Hyde <[email protected]> wrote: > 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 >
