New PR template #3255
New PR template #3255
Conversation
cprecioso
temporarily deployed
to
fly-deploy-test
GitHub Actions
Inactive
cprecioso
had a problem deploying
to
fly-deploy-test
GitHub Actions
Error
cprecioso
temporarily deployed
to
fly-deploy-test
GitHub Actions
Inactive
| - Bug fix: patch version (0.0.X). | ||
| - New feature: patch version (0.0.X). | ||
| - Breaking change: minor version (0.X.0). | ||
| If the version has already been bumped since the last release, you can skip this. |
There was a problem hiding this comment.
I'm not sure if this is effective without being overbearing. Worst case scenario, we can just tell them to undo a change, or we can do it ourselves.
There was a problem hiding this comment.
Ok so I like it all together, nice tone, and it is certainly an improvement, but I am tempted to try to push it even further.
I was doing a PR yesterday and current checklist felt overwhelming, and while this one is better, I think it will stil feel similar in many ways, because it is so much text + we have to be making decisions. Making decisions is the hardest part really. What kind of change have I made? Have I added or updated a feature?
Any ideas how we can accomplish this?
What if we had "flows" for different situations: if you did just a bug fix, you get one flow. If you did a feature, you get another. Yeah there would be duplication, but might be worth it DX-wise.
Maybe we can achieve this with multiple PR templates, I think you mentioned taht as an idea. Or, we have it here, collapsed, and ask them to fill it in preview mode? I just tried it, they can switch to preview mode immediately when creating PR. We can tell them, hey go fill ti in preview mode for nicest experience.
| <!-- | ||
| Describe your PR! | ||
| 1. [ ] I added a regression test that reproduces the bug and verifies the fix. | ||
| Common questions we'd like you to answer: | ||
| - What's the motivation for this change? | ||
| - Which changes are included in this PR? | ||
| - If there are many different changes, consider splitting your PR into smaller | ||
| ones. It will go through faster! | ||
| If you're unable to add a regression test, please explain why. | ||
| This likely indicates that our current testing setup needs improvement. | ||
| If this PR closes an issue, write “Fixes #XXXX" so GitHub will link them together. | ||
| ### Test Coverage | ||
| You can also answer some of these questions if they are relevant: | ||
| - Does this change affect users? How? | ||
| - Have you considered any other approaches? Why is this one the best? | ||
| - Are there any drawbacks or edge cases? | ||
| - What are the possibilities for future work? | ||
| --> |
There was a problem hiding this comment.
So are they supposed to replace this comment with description, or write under it, or not to anything here and that is it? This would be confusing to me I think, where am I suppoed to write this exactly, or am I at all.
There was a problem hiding this comment.
You can do either, the comment doesn't affect anything. It can be safely removed or safely kept. That's why there's no specific guidance.
There was a problem hiding this comment.
Ok so I get your explanation, but still, my feeling was as I described above: I didn't know waht is expected, and that is fatiguing. Would be great if we can somehow remove that feeling of "what do I need to do here", even if all the choices are correct ones. Not sure if we can easily do it though, in that case I guess we stick with what we have, but I am challenging it.
.github/pull_request_template.md
Outdated
|
|
||
| 1. [ ] I updated [`waspc/examples/todoApp`](https://github.com/wasp-lang/wasp/tree/main/waspc/examples/todoApp) and its e2e tests as needed and manually checked it works correctly. | ||
| - [ ] I tested this change in a Wasp app **locally**. | ||
| - [ ] <!-- If you modified Haskell code: --> I added **unit tests** for my change at `waspc/tests`. |
There was a problem hiding this comment.
So I would even mind these comments not being comments: as a reviewer, I will also be reading this PR template, and this info is useful, that this is noly important if you modified haskell code.
There was a problem hiding this comment.
Mmm I had the opposite reaction, as a reviewer, I want to be able to quickly scan through what was done and not without having extra noise :/
There was a problem hiding this comment.
But then you need to know fo each of these checkboxes what is the condition under which they need to behcked, and you need to mentally go th rough that + remember.
I would rather have it explicit then, Idon't want to think about it each time, trying to remember what happens when.
You can't edit text or check boxes in preview mode :/ |
Ah uff ok that sucks, I haven't tried actually, I assumed you can toggle the checkboxes at least! And text yeah, I knew you can't but forgot that is going to be an issue. Blargh. Maybe we go route of multiple PR templates, what do you think? I know I am stating what problems are, without neccessarily offering solid solutions, but let's not give up yet. Btw what are others doing? Nobody has such extensive checklists embedded in the PR tempalte I guess, they probably have them somewhere else, if they have them? |
So, I checked, and having multiple PR templates is not really possible, it needs to be a single one to get autofilled by GH. I checked a lot of PR templates of popular OSS projects and most have very light PR templates, they're mostly something like: <!--
Welcome!
- Add an explanation of your changes and the motivation for them.
- Is it a breaking change?
- Remember to add tests for your changes and explain what is affected.
- Remember to add a changelog entry
You can link a PR to an issue by writing "Fixes #XXX"
-->Others just point to their contributing guidelines. The most complete checklist I have found is Storybook's, and I'm not sure if it's relevant here https://github.com/storybookjs/storybook/blob/next/.github/PULL_REQUEST_TEMPLATE.md?plain=1 I don't think we have a lot of guidance, it seems other projects are not solving the problems we want through the PR template. |
cprecioso
temporarily deployed
to
fly-deploy-test
GitHub Actions
Inactive
Ok yeah the one from storybook seems a bit similar! Maybe others are not that strict in their process, or even have lists somewhere else, or don't have so many interconnected parts of the project as we do, or who knows what. Would be interesting to investigate that also I guess, but not sure if it is worth doing it now. |
|
@cprecioso seems to me the main problem remains -> there is a lot of stuff in the template, and it is overwhelming, both for the author and for the reviewer. But doesn't seem like we have any solutions on the table? We can't have multiple PR templates, we can't have them edit in preview mode, what is left? Some ideas that come to my mind:
|
|
Ok so since I was saying a lot that I would like it to be better but wasn't sure how, I gave it a bit of a try myself, to see what happens if I go at it for a bit. I tried looking from two sides: what I need as a person filling it in (author), and I need as a reviewer. The main thing I felt was problematic as an author was that there was too much to read. I know it is in comments, but is still a lot, it is overwhelming. From the reviewer side, I found it still somewhat hard to evaluate the filled in stuff -> I can see they haven't left the checkmarks on some stuff, but how do I know if they just skipped it, forgot it, should have done it but didn't yet, ...? One thing I still don't like is that when checkbox is empty, it is not clear if that is on purpose, or by accident, or what. That is why I suggested they strikethrough the stuff that is not relevant for them. That is great for reviewer, but it could be a bit cumbersome for author though, having to strikethrough stuff in markdown is a bit involved, so I am not super sure about that. p.s. that thing with semver in front of "type of change" bullets -> that is a bit of experiment, I am nto sure that is the best way, but it is how I tink about it in my head, how I "translate" it to myself -> aha, ok so this is no version number change, this is patch update, this is minor update, this is major version update. So I thought making having it explicit like that can help. <!--
Thanks for contributing to Wasp!
Make sure to follow this PR template, so that we can speed up the review process.
It will also help you not forget important steps when making a change.
If you don't know how to fill any of the sections below, it's okay to leave them blank,
we will help you out during the review.
-->
## Description
<!-- Write a high-level overview + any additional context (motivation, trade-offs, approaches considered, concerns, ...) -->
TODO
## Type of the change
<!-- Select just one, the largest change: -->
- [ ] `v _._._` **Just code/docs improvement** <!-- no functional change -->
- [ ] `v _._.+1` **Bug fix** <!-- non-breaking change which fixes an issue -->
- [ ] `v _.+1.0` **New/improved feature** <!-- non-breaking change which adds functionality -->
- [ ] `v +1.0.0` **Breaking change** <!-- fix or feature that would cause existing functionality to not work as expected -->
## Checklist
<!-- Check the relevant boxes, and strikethrough those that are not. -->
- 🧪 Tests:
- [ ] I added **unit tests** for my change. <!-- If not, explain why. -->
- [ ] _[If you fixed a bug]_ I added a **regression test** for the bug I fixed. <!-- If not, explain why. -->
- [ ] _[If you added/updated a feature]_ I added/updated **e2e tests** at `waspc/examples/todoApp/e2e-tests`.
- [ ] _[If you added/updated a feature]_ I updated the **starter templates** at `waspc/data/Cli/templates`, as needed.
- 📜 Documentation:
- [ ] _[If you added/updated a feature]_ I **added/updated the documentation** in `web/docs/`.
- 🆕 Changelog:
- _[If change is more than just code/docs improvement]_
- [ ] I updated `waspc/ChangeLog.md` with a **user-friendly** description of the change.
- [ ] I **bumped the `version`** in `waspc/waspc.cabal` to reflect the changes I introduced.
- [ ] _[If you did a breaking change]_ I added a step to the current **migration guide** at `web/docs/migration-guides/`.
<!--
On updating the waspc version in waspc/waspc.cabal:
While we're pre-1.0, the version bumping follows a bit of special rules:
- Bug fix or new feature: 0.X.(Y+1).
- Breaking change: 0.(X+1).0.
where 0.X.Y is the version of the last release.
If the version has already been bumped as needed on `main` branch since the last release, skip this.
--> |
|
And here is a different attempt, where I went with duplication, in order to achieve separate flow for different situations. That said, while doing it, I realized Anyway, here is the md for this, I kind of like it in the sense that it is easier to reason about it, no IFs in it, but there is duplication which makes maintainance somewhat harder (might be ok for us though) and also they need to delete some headers but that doesn't sound terrible to me. p.s. I quite liked the emojis I used here for different change types, I think they are quite standard so help with quickly grasping: wrench, bug, rocket, explosion. So we could use those in any case. <!--
Thanks for contributing to Wasp!
Make sure to follow this PR template, so that we can speed up the review process.
It will also help you not forget important steps when making a change.
If you don't know how to fill any of the sections below, it's okay to leave them blank,
we will help you out during the review.
-->
## Description
<!-- Write a high-level overview + any additional context (motivation, trade-offs, approaches considered, concerns, ...) -->
TODO
## Checklist
<!--
Below are 4 different checklists, one for each type of PR regarding what level of change it introduces:
- just code/docs improvement (no version bump)
- bug fix (patch version bump)
- new/improved feature (minor version bump)
- breaking change (major version bump)
Identify which one your PR falls under and fill it, while deleting the other checklists.
-->
### 🔧 Just code/docs improvement (`v _._._`)
- 🧪 Tests:
- [ ] I added **unit tests** for my change. <!-- If not, explain why. -->
### 🐞 Bug fix (`v _._.+1`)
- 🧪 Tests:
- [ ] I added **unit tests** for my change. <!-- If not, explain why. -->
- [ ] I added a **regression test** for the bug I fixed. <!-- If not, explain why. -->
- 🆕 Changelog:
- [ ] I updated `waspc/ChangeLog.md` with a **user-friendly** description of the change.
- [ ] I **bumped the `version`** in `waspc/waspc.cabal` to reflect the changes I introduced.
- We are pre-1.0, so you should bump it as `0.X.(Y+1)` where `0.X.Y` is version of the last Wasp release.
If it is already bumped on `main`, you don't have to do anything.
### 🚀 New/improved feature (`v _.+1.0`)
- 🧪 Tests:
- [ ] I added **unit tests** for my change. <!-- If not, explain why. -->
- [ ] I added/updated **e2e tests** at `waspc/examples/todoApp/e2e-tests`.
- [ ] I updated the **starter templates** at `waspc/data/Cli/templates`, as needed.
- 📜 Documentation:
- [ ] I **added/updated the documentation** in `web/docs/`.
- 🆕 Changelog:
- [ ] I updated `waspc/ChangeLog.md` with a **user-friendly** description of the change.
- [ ] I **bumped the `version`** in `waspc/waspc.cabal` to reflect the changes I introduced.
- We are pre-1.0, so you should bump it as `0.X.(Y+1)` where `0.X.Y` is version of the last Wasp release.
If it is already bumped on `main`, you don't have to do anything.
### 💥 Breaking change (`v +1.0.0`)
- 🧪 Tests:
- [ ] I added **unit tests** for my change. <!-- If not, explain why. -->
- [ ] I added/updated **e2e tests** at `waspc/examples/todoApp/e2e-tests`.
- [ ] I updated the **starter templates** at `waspc/data/Cli/templates`, as needed.
- 📜 Documentation:
- [ ] I **added/updated the documentation** in `web/docs/`.
- 🆕 Changelog:
- [ ] I updated `waspc/ChangeLog.md` with a **user-friendly** description of the change.
- [ ] I **bumped the `version`** in `waspc/waspc.cabal` to reflect the changes I introduced.
- [ ] I added a step to the current **migration guide** at `web/docs/migration-guides/`.
- We are pre-1.0, so you should bump it as `0.(X+1).0` where `0.X.Y` is version of the last Wasp release.
If it is already bumped on `main`, you don't have to do anything. |
There was a problem hiding this comment.
@cprecioso ok, so I reviewed it again, and also ended up proposing two alternative takes. I felt like I was just asking can it be better, without offering any solutions, and I knew no other way but to try it on my own, so I ended up with these -> check them out, let's see what we can use.
We might also want to soon have others look at this PR, everybody will be using the checklist so it would be good to have a buy-in + ideas from them also.
Description
Updates the PR template. I tried to get a nice point in the middle of being comprehensive but not being too overloading, so that (both external contributors and us) don't get too lazy to fill it up, but still get reminded to check all the necessary things.
Select what type of change this PR introduces:
Checklist
waspc/tests.waspc/examples/todoApp/e2e-tests.waspc/data/Cli/templates, if needed.web/docs/, in a place that makes sense.web/docs/and updated them, if needed.waspc/ChangeLog.mdwith a user-friendly description of the change.versioninwaspc/waspc.cabalto reflect changes I introduced.web/docs/migration-guides/.