-
Notifications
You must be signed in to change notification settings - Fork 9
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
blog/code-review-stacked-prs/ #37
Comments
liked this approach, will follow it now onwards! 👌🏻 thanks for writing 🙌🏻 |
Gem of a post man, wish could have found it sooner! However, in hindsight of working with big features we oftentimes have to update our PRs with requested changes, which can be needed on its subsequent PRs as well. Does this mean we'd have to rebase subsequent branches with every new update to former PRs? 🤔 |
@heytulsiprasad generally the requested changes don't (or shouldn't) require changing UI functionality. Mostly they would be for code style, readability, etc. In that case I would push such changes on the same PR (without rebasing this change onto subsequent stacked PRs). For functional changes you could create a new stacked PR on top of all PRs. However, say you need functional changes to be reflected on all stacked PRs then it would need a rebase on all subsequent branches unfortunately. |
Thank you @divyanshu013, I've always neglected to use stacked PRs for such reason, but I'll try with giving review fixes in yet another PR. However, this has more upsides than down so definitely worth it. |
Stacked Pull Requests | Divyanshu Maithani
Effective code review on merging large changes with stacked PRs
https://divyanshu013.dev/blog/code-review-stacked-prs/
The text was updated successfully, but these errors were encountered: