-
-
Notifications
You must be signed in to change notification settings - Fork 1.4k
New PR template #3255
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
Open
cprecioso
wants to merge
7
commits into
main
Choose a base branch
from
cprecioso/3190-update-our-pr-template-automatic-verification
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
+60
−39
Open
New PR template #3255
Changes from all commits
Commits
Show all changes
7 commits
Select commit
Hold shift + click to select a range
3e913ec
New PR template
cprecioso 90d4c96
Indent
cprecioso bb8f4fb
Tweaks
cprecioso 5aa1b69
fix
cprecioso fb27b20
Merge branch 'main' into cprecioso/3190-update-our-pr-template-automa…
cprecioso d47bcb0
Update
cprecioso d9382b2
Fix
cprecioso File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,62 +1,83 @@ | ||
| ### Description | ||
|
|
||
| > Describe your PR! If this PR closes an issue, use “Fixes #(issue_number)" syntax so GitHub will auto-close it when merged. | ||
|
|
||
| ### Select what type of change this PR introduces: | ||
| <!-- | ||
| Hi, thanks for contributing to Wasp! | ||
|
|
||
| 1. [ ] **Just code/docs improvement** (no functional change). | ||
| 2. [ ] **Bug fix** (non-breaking change which fixes an issue). | ||
| 3. [ ] **New feature** (non-breaking change which adds functionality). | ||
| 4. [ ] **Breaking change** (fix or feature that would cause existing functionality to not work as expected). | ||
| Comments like this one won't be shown in the final PR, but they contain | ||
| instructions to guide you. | ||
|
|
||
| ### Update Waspc ChangeLog and version if needed | ||
| 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 did a **bug fix, new feature, or breaking change**, that affects `waspc`, make sure you satisfy the following: | ||
| 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. | ||
| --> | ||
|
|
||
| 1. [ ] I updated [`ChangeLog.md`](https://github.com/wasp-lang/wasp/blob/main/waspc/ChangeLog.md) with description of the change this PR introduces. | ||
| 2. [ ] I bumped `waspc` version in [`waspc.cabal`](https://github.com/wasp-lang/wasp/blob/main/waspc/waspc.cabal) to reflect changes I introduced, with regards to the version of the latest wasp release, if the bump was needed. | ||
|
|
||
| ### Add a regression test if needed | ||
| ### Description | ||
|
|
||
| If you did a **bug fix**, make sure you satisfy the following: | ||
| <!-- | ||
| 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? | ||
| --> | ||
|
|
||
| Please ensure your changes are adequately tested: | ||
| ### Select what type of change this PR introduces: | ||
|
|
||
| 1. [ ] **My changes are covered by tests** (unit, integration, or e2e tests as appropriate). | ||
| <!-- Put an x in between the brackets to select options, like so: [x] --> | ||
cprecioso marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
|
||
| If you're unable to add tests or if coverage is partial, please explain why below: | ||
| - [ ] **Just code/docs improvement** (no functional change). | ||
| - [ ] **Bug fix** (non-breaking change which fixes an issue). | ||
| - [ ] **New feature** (non-breaking change which adds functionality). | ||
| - [ ] **Breaking change** (fix or feature that would cause existing functionality to not work as expected). | ||
|
|
||
| <!-- Provide explanation here if tests are missing or incomplete --> | ||
| ### Checklist | ||
|
|
||
| ### Update example apps if needed | ||
| <!-- Put an x in between the brackets to select options, like so: [x] --> | ||
| <!-- You can add notes or explanations wherever needed --> | ||
|
|
||
| If you did code changes and **added a new feature**, make sure you satisfy the following: | ||
| - 🧪 Testing: | ||
cprecioso marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
|
||
| 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**. | ||
| - [ ] I added **unit tests** for my change at `waspc/tests`. | ||
| - [ ] <!-- If you added or updated a feature: --> I added **e2e tests** for my change at `waspc/examples/todoApp/e2e-tests`. | ||
| - [ ] <!-- If you added or updated a feature: --> I updated the **starters** at `waspc/data/Cli/templates`, if needed. | ||
| - [ ] <!-- If you fixed a bug: --> I added a **regression test** for the bug I fixed. | ||
|
|
||
| If you did code changes and **updated an existing feature**, make sure you satisfy the following: | ||
| - 📜 Documentation: | ||
|
|
||
| 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. | ||
| - [ ] <!-- If you added a feature: --> I **added documentation** to the `web/docs/`, in a place that makes sense. | ||
| - [ ] <!-- If you updated a feature: --> I **searched** for all relevant places in the `web/docs/` and **updated** them, if needed. | ||
|
|
||
| ### Update starter apps if needed | ||
| - 🆕 Changelog: | ||
|
|
||
| If you did code changes and **updated an existing feature**, make sure you satisfy the following: | ||
| <!-- If you did a bug fix, new feature, or breaking change: --> | ||
|
|
||
| 1. [ ] I updated [starter skeleton](https://github.com/wasp-lang/wasp/tree/main/waspc/data/Cli/templates/skeleton) as needed and manually checked it works correctly. | ||
| 2. [ ] I updated [`basic` starter](https://github.com/wasp-lang/wasp/tree/main/waspc/data/Cli/templates/basic) as needed and manually checked it works correctly. | ||
| 3. [ ] I updated [`todo-ts` starter](https://github.com/wasp-lang/starters/tree/dev/todo-ts) as needed and manually checked it works correctly. | ||
| 4. [ ] I updated [`embeddings` starter](https://github.com/wasp-lang/starters/tree/dev/embeddings) as needed and manually checked it works correctly. | ||
| 5. [ ] I updated [`saas` starter](https://github.com/wasp-lang/open-saas/tree/main/template) as needed and manually checked it works correctly. | ||
| - [ ] I updated `waspc/ChangeLog.md` with a **user-friendly** description of the change. | ||
| - [ ] I **bumped the `version`** in `waspc/waspc.cabal` to reflect changes I introduced. | ||
| - [ ] <!-- If you did a breaking change: --> I added a step to the current **migration guide** at `web/docs/migration-guides/`. | ||
|
|
||
| ### Update e2e tests if needed | ||
| <!-- | ||
| While we're in beta, the version should be bumped according to the type of change: | ||
| - 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
| --> | ||
|
|
||
| If you did code changes and changed Wasp's code generation logic, make sure you satisfy the following: | ||
| <!-- | ||
| Thanks for contributing! :) | ||
|
|
||
| 1. [] I updated [e2e tests](https://github.com/wasp-lang/wasp/tree/main/waspc#end-to-end-e2e-tests) as needed and manually checked they are correct. | ||
| We'll check this PR as soon as we can. Meanwhile, keep an eye on this PR and | ||
| fix any errors that might come up in the checks. It should be all green before | ||
| we can merge it. | ||
| --> | ||
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.