-
Notifications
You must be signed in to change notification settings - Fork 334
doc: before-requesting-review checklist (AGENTS.md + ErdosProblems README) #4378
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
base: main
Are you sure you want to change the base?
Changes from 3 commits
5e1d39a
9feae26
54521f8
434387e
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -461,6 +461,8 @@ Before considering your work complete, verify: | |||||
| - [ ] Docstrings present for main definitions | ||||||
| - [ ] `research open`, `textbook` and `research solved` docstrings include a concise description | ||||||
| - [ ] Code properly formatted and readable | ||||||
| - [ ] Searched `FormalConjecturesForMathlib` and neighboring files for existing definitions, API, and notation before adding new ones | ||||||
| - [ ] Notes to reviewers (formalization choices, caveats) are in the PR description, not the Lean file | ||||||
|
Collaborator
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.
Suggested change
|
||||||
|
|
||||||
| ### Testing Definitions | ||||||
|
|
||||||
|
|
@@ -490,6 +492,8 @@ theorem myNewClass_sanity_check : | |||||
| - Add large proofs (this is a benchmark repository, not a proof repository) | ||||||
| - Use camelCase for theorem names | ||||||
| - Create placeholder definitions in FormalConjecturesForMathlib/ | ||||||
| - Redefine notation or API that already exists in `FormalConjecturesForMathlib` or a neighboring problem file | ||||||
|
Collaborator
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.
Suggested change
|
||||||
| - Put meta-commentary or notes-to-reviewers in Lean files (they belong in the PR description) | ||||||
|
|
||||||
| ✅ **DO:** | ||||||
|
|
||||||
|
|
||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -48,3 +48,28 @@ An example of this would be: | |
| - [Va99] Various, Some of Paul's favorite problems. Booklet produced for the conference "Paul Erdős | ||
| and his mathematics", Budapest, July 1999 (1999). | ||
| ``` | ||
|
|
||
| ## Before requesting review | ||
|
|
||
| A checklist of the corrections that come up most often in review. Ticking | ||
| these off first (yourself, or whatever agent prepared the PR) usually saves a | ||
| review round trip: | ||
|
|
||
| 1. **Docstrings quote the website verbatim.** Copy the LaTeX from | ||
| erdosproblems.com rather than rephrasing it. Only deviate to fix a genuine | ||
| error or inaccuracy in the original formulation. | ||
| 2. **Solved problems cite the solution.** The text below the problem box | ||
| typically explains who solved it and in which paper; copy that sentence | ||
| verbatim into the docstring as well. | ||
| 3. **Search before defining.** Check `FormalConjecturesForMathlib` and | ||
| neighboring problem files for existing definitions, API, and notation | ||
| before introducing your own. If something you need seems generally useful, | ||
| it belongs in `FormalConjecturesForMathlib`, not inlined in the problem | ||
| file. | ||
|
Collaborator
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. This sould go to AGENTS.md: it is not ErdosProblems specific |
||
| 4. **Keep commentary out of the Lean file.** Notes to reviewers, formalization | ||
| choices, and caveats belong in the pull request description, not the | ||
|
Collaborator
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. some formalisation choices are probably good to keep in the docstrings/comments |
||
| source. | ||
|
Collaborator
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. also should go to AGENTS.md |
||
| 5. **Use LaTeX, not backticks, for mathematics in docstrings.** See | ||
| `AGENTS.md` for the exceptions on the Lean-API side. | ||
|
Collaborator
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. Probably not worth repeating here? |
||
| 6. **Add sanity tests for new definitions.** A nontrivial definition should | ||
| come with at least one `@[category test]` statement exercising it. | ||
|
Collaborator
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. also should go to AGENTS. |
||
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.