Update PR template for monorepo #602
Conversation
|
This pull request is being automatically deployed with Vercel (learn more). 🔍 Inspect: https://vercel.com/showtime/showtime/5bDK5XNXSFzQ8h5zSax7gv7m5vfR |
axeldelafosse
requested review from
fontanierh,
intergalacticspacehighway,
m1guelpf and
TatisLois
| @@ -0,0 +1,17 @@ | |||
| # Why | |||
There was a problem hiding this comment.
If the consensus is to remove the impact section then I’m for it but I’ll offer a counter point of view!
For the monorepo I think the impact section is useful. It’s not always clear if a change will affect mobile, web, design system or another app/package since they are interwoven in different ways.
Further highlighted by the fact that we don’t have any automated tests to keep us somewhat guarded.
I also think it’ll help reviewers know how and where to test a PR.
+1 on your slack comment about omitting sections that don’t apply. We can use our best judgment!
There was a problem hiding this comment.
Looks good. Also, we can add a checklist. web, ios, android, dark mode. It can also be a part of linear issue though.
There was a problem hiding this comment.
It’s not always clear if a change will affect mobile, web, design system or another app/package since they are interwoven in different ways.
This should be highlighted in the "Why" imo
I also think it’ll help reviewers know how and where to test a PR.
And this should be in the "Test Plan"
It can also be a part of linear issue though.
Yeah good idea! It won't always be a universal change (at least for now) so let's keep it simple for everyone.
There was a problem hiding this comment.
This should be highlighted in the "Why" imo
actually this is exactly the purpose of the “impact” section :D
There was a problem hiding this comment.
Haha yeah I know, I meant to say that a good "Why" section should highlight this.
There was a problem hiding this comment.
Not sure something like "Open PRs that use components XYZ will need to be rebased before merging" really fit in "Why" though
There was a problem hiding this comment.
Agreed. Feel free to add an "Impact" section when there is a comment like this! I just feel like it's not going to happen much at our stage and I want to tend towards simplicity.
There was a problem hiding this comment.
imho the goal of having it in the template is to force the person opening the PR to think about it. If it's not in the template, then it's very easy to forget about the potential impact of your PR
|
Here is my proposal: let's keep it simple and see how our team is using this template. Feel free to add (or remove) any section. We will be able to revisit this template later on based on our usage. |
Sorry if I misunderstood but why merge the changes if we are taking a wait and see approach based on usage? Why not have left it and remove it. |
|
Feel free to add it back but my goal is to avoid adding more complexity to the process. I think we should default to simplicity. |
Nah, that would be more work than it's worth haha. What's done is done 🙂 |
Why
Wanted to simplify the template for this monorepo, I think we don't need the "Impact" section.
How
Took the template from Expo https://github.com/expo/expo/blob/master/.github/PULL_REQUEST_TEMPLATE