Thanks to Julian and Alessandro for your reply.I agree with your point,maybe I don't have a clear description of my thought.
Because we have a forced update of the main branch before, now almost every PR on github has hundreds of File changes and dozens of commits, so it is a bit difficult to review. What I'm trying to say is that the author of the PR could rebase the latest code in the main branch, recommit, and the records will be back to normal. [image: 20230907-095212.jpeg] Best, LakeShen Alessandro Solimando <alessandro.solima...@gmail.com> 于2023年9月7日周四 00:41写道： > 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 <jhyde.apa...@gmail.com> 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 <jhyde.apa...@gmail.com> > 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 <shenleifight...@gmail.com> > 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 > > >
