shunping commented on PR #33611: URL: https://github.com/apache/beam/pull/33611#issuecomment-2631443995
> Hi @shunping, > > Thanks for your follow-up review! I've addressed your feedback. > > This might be unrelated, but I’d like to use this PR as an opportunity to learn more about Beam development practices as I am getting used to it. My interest was sparked by this discussion: [#33672 (comment)](https://github.com/apache/beam/pull/33672#issuecomment-2630967193). > > When addressing a review and make a follow-up commit to fix the issue, Someone should avoid squashing reviewed and unreviewed commits. After the follow-up review is completed, would it be necessary to squash the follow-up commit (I am thinking about delay in the merging process e.g for the CI to be triggered again), or is it acceptable to leave the follow-up commit as part of the commit history? The root problem here is about modifying an "already pushed" commit. In your previous PR and this one as well, you pushed your code to github and then you did an operation locally to modify your commit history (eg. squashing the commits or simply amending an already pushed commit). You may notice that, when you pushed your code again, git reminded you to do a "forced" push, which is then shown up in github as something like: ``` mohamedawnallah force-pushed the enableRecursiveDeleteGCS branch from 661f822 to 3491c70 ``` This is not a good practice for code that is already in the reviewing pipeline. Specifically, when you force-push a commit (which changes the original commit sha1), github can no longer associate it with the reviews that was previously submitted to it. As a result, it will be inconvenient for reviewers to go back and forth to compare what you had changed and what were the previous feedback. Check the following pages for some more reading. - https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/proposing-changes-to-your-work-with-pull-requests/about-pull-requests#about-pull-requests - https://jvns.ca/blog/2023/11/06/rebasing-what-can-go-wrong-/#force-pushing-makes-code-reviews-harder -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
