Skip to content

Commit

Permalink
doc: Update PR checklist.
Browse files Browse the repository at this point in the history
  Resolves numberscope#174.
  • Loading branch information
gwhitney committed Aug 30, 2024
1 parent de5f16c commit 3ca1474
Showing 1 changed file with 21 additions and 11 deletions.
32 changes: 21 additions & 11 deletions doc/pull-request-checklist.md
Original file line number Diff line number Diff line change
Expand Up @@ -5,13 +5,18 @@
The PR submitter should:

- Please use a `snake_case` name for the branch in the PR.
- Please limit your PR's focus to one thing.
- Please limit the PR's focus to one thing.
- Ideally, submit a small PR with limited functionality that can be built
upon instead of submitting one huge, monolithic PR. Typically, the smaller
the PR, the better, with the obvious caveat that the code in your PR needs
the PR, the better, with the obvious caveat that the code in the PR needs
to work and actually do something. A small PR for a visualizer might be in
the ballpark of 200 lines of code.
- Write or supplement test(s) for the file(s) you touch in your PR.
- Write or supplement test(s) for the file(s) affected by the PR. In
particular, if it is a bug fix PR, there **must** be a new test that would
fail with the prior code, but passes in the PR. The new tests may be
either unit (vitest) or end-to-end (Playwright) tests or both, as
appropriate.
- Update all documentation to reflect the changes in the PR.
- Make sure Numberscope runs properly, including former supposedly
unmodified behaviors and newly implemented ones. In particular, run it via
'npm run dev' and with the browser console open and make sure there are no
Expand All @@ -22,19 +27,24 @@ The PR submitter should:
## For PR reviewers

- All new or changed features are appropriately documented.
- Tests are appropriately modified for all new or changed features.
- Tests are appropriately modified for all new or changed features. If it is
a bugfix PR there must be at least one new test. Most other PRs should
have new or changed tests as well. So if you do not see any changed files
in the e2e directory or in any **tests** directory, that is at least an
"orange" flag.
- The PR is passing lint by running `npm run lint`. There should be no
changed files and no warnings/errors.
- The PR passes all tests. Note the GitHub CI infrastructure will check this
for you.
- The PR builds by running `npm run build`. (This also checks type
correctness.) There should be no errors, and for now the only allowed
warning is the one about some assets being too big.
- The PR passes all tests. Right now (Oct. 2022), just by running
`npm run test:unit`.
- Numberscope runs properly -- basically the same check as on the submitter
list, but be sure to exercise as many randomly selected behaviors as you
have time for, definitely including but not limited to the ones nominally
affected by the PR. This should be done in `npm run preview` mode after a
successful build.
- The built system runs with `npm run preview`.
- When run that way, Numberscope operates properly. The end-to-end tests
take the pressure to try a variety of random behaviors off of the
reviewer, but a number of possible actions that seem related to the
changes in the PR must definitely be tried in this fashion before
approving for merge.
- At the end of the review process, before merging, add a commit to update
the
["Contributors" section of the "About" document](about.md#contributors) to
Expand Down

0 comments on commit 3ca1474

Please sign in to comment.