New PR template - #1260
Conversation
Pyright Type CompletenessView the full Project (full
Other symbols referenced but not exported by
Symbols without documentation:
Patch (exported symbols added or changed by this PR): no exported symbol type-completeness changes detected. |
|
@genedan @henrydingliu you guys had a comment in #1043 to add type hinting in the PR template, where is the best place for this? I feel like if we just add it to the checklist it's awkward. |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1260 +/- ##
==========================================
+ Coverage 91.70% 91.90% +0.19%
==========================================
Files 93 93
Lines 5438 5608 +170
Branches 699 737 +38
==========================================
+ Hits 4987 5154 +167
- Misses 327 328 +1
- Partials 124 126 +2
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
what would awkward about adding it to the checklist? |
Type hinting to me is not the same "level" as the other checklist items above, and it's also already in the Governing Doc, under the PRs "should": "Include proper type hinting". Because it's not a "must", I don't know that adding it as a checklist item is a good idea? Plus, we already have the first item that you have read the Governing Doc and will adhere to it, though I'm not sure if that's too loose since the doc is very long. What is your opinion? |
type hinting is a required check that's currently xfail. up to you if you want to hit it with this PR template revision |
|
I think we leave it off for now. I'm sure there will be a day that we require all implementations to be properly typed and include the docstrings/examples. When we are there we can add it then is my vote. |
| [TST] for unit testing | ||
| [CHORE] for chores and maintenance tasks | ||
| [BRK] for breaking changes, deprecations, and removals | ||
| - [ ] I am a human (not a bot), and this PR is not AI generated. |
There was a problem hiding this comment.
I recommend replacing "this PR" to "this PR description", since we do allow LLMs for code, but discourage them for the communications.
There was a problem hiding this comment.
Take a look at the new language, is that better?
| [DOCS] for documentation | ||
| [TST] for unit testing | ||
| [CHORE] for chores and maintenance tasks | ||
| [BRK] for breaking changes, deprecations, and removals |
There was a problem hiding this comment.
I'm on the fence with having the whole list of tags in the template. It is possible to have an automatic check that fails unless the tag is present, what do you think?
There was a problem hiding this comment.
How about we compromise? I think it's good to have a short list here just so you don't have to always look up what the actual tag is and what's available?
| <!-- Do not edit anything below until the ticket is open. Checklists below. --> | ||
|
|
||
| ## Submitter's Checklist | ||
| - [ ] I have reviewed and am adhering to the standards outlined in the project Governing Doc. |
There was a problem hiding this comment.
Add a hyperlink to the Governing Doc.
|
|
||
|
|
||
| ## Checklist | ||
| - [ ] I passed tests locally for both code (`uv run pytest`) and documentation changes (`uv run --directory docs jb build . --builder=custom --custom-builder=doctest`) |
There was a problem hiding this comment.
I'd recommend adding a bullet Submitter's Checklist that replaces this one. We now have a pre-commit hook that can one-shot all the workflows locally:
I passed all pre-commit checks prior to push (with a link to the list of workflows we deploy in the Governing Doc).
There was a problem hiding this comment.
Do you think this is necessary? We already have the CI tests that run automatically, and there's also the last bullet on the reviewer's list "CI tests passed or failure are acceptable"
|
I think it's OK to have a checklist item for it, although I'm not super adamant about it. Eventually we will be able to enforce it automatically and won't have the checklist item anymore. Right now the % coverage has been moving up despite not having the checkbox, and I personally don't nag people about it during reviews (unlike my comments on spacing and indentation). If you find that you are constantly reminding people in your reviews to put the hints in, you should have the checkbox. |
|
Maybe with typing, we can break up the submitter's list? So we have
Or would this be too long? And also this is turning from a "should" to a "must". |
3 participants
Summary of Changes
Updated the PR template in accordance with the governing doc
Related GitHub Issue(s)
Closes #1240
Additional Context for Reviewers
There's not really a way for me to preview this, so I hope it goes well. Please let me know if something looks weird (don't know how to preview)
Checklist
uv run pytest) and documentation changes (uv run --directory docs jb build . --builder=custom --custom-builder=doctest)Note
Low Risk
Process-only change to the GitHub PR template with no runtime or library code impact.
Overview
Updates
.github/pull_request_template.mdso new PRs follow the project governing doc instead of the old single local-test checklist.Adds an AI/LLM Usage section (with disclosure guidance), a hint on issue-linking keywords, and splits review into Submitter's and Reviewer's checklists covering governance adherence, title prefixes (
[FIX],[FEAT], etc.), human attestation,ARCHITECTURE.md, linked issues, AI disclosure, docs/tests, reviewers, and CI. Section order and boilerplate comments are adjusted; the prior checklist item foruv run pytest/ docs doctest is removed in favor of the governance-aligned items.Reviewed by Cursor Bugbot for commit 2015105. Bugbot is set up for automated code reviews on this repo. Configure here.