-
-
Notifications
You must be signed in to change notification settings - Fork 769
add Copilot PR review instructions #19738
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: master
Are you sure you want to change the base?
Changes from 2 commits
b7bf5a7
4e09892
fc06662
34a72d9
7bb947f
fd2c30c
13c82b8
f934ab5
d92a637
68e5661
8c6dcbf
dafec43
23b93b5
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 | ||||
|---|---|---|---|---|---|---|
| @@ -0,0 +1,64 @@ | ||||||
| # Copilot instructions for GitHub code reviews (NVDA) | ||||||
|
|
||||||
| Use these instructions when reviewing pull requests in this repository. | ||||||
|
|
||||||
| ## Review goals | ||||||
|
|
||||||
| * Prioritize correctness, regressions, accessibility impact, API compatibility, and security. | ||||||
| * Keep feedback specific and actionable; prefer concrete code suggestions over generic comments. | ||||||
| * Classify issues by severity: | ||||||
| * **Blocking**: likely bug, regression risk, security concern, API break, missing required tests/docs. | ||||||
|
seanbudd marked this conversation as resolved.
|
||||||
| * **Non-blocking**: style consistency, readability, minor refactors. | ||||||
| * Avoid requesting unrelated refactors or broad redesigns outside PR scope. | ||||||
|
|
||||||
| ## NVDA Python style checks | ||||||
|
|
||||||
| When reviewing Python changes, verify alignment with `projectDocs/dev/codingStandards.md`: | ||||||
|
|
||||||
| * Follow PEP 8 unless NVDA guidance differs. | ||||||
|
seanbudd marked this conversation as resolved.
Outdated
|
||||||
| * **Indentation must use tabs**, not spaces. | ||||||
| * New/changed code should include **PEP 484 type hints** (prefer `X | Y` and `T | None`). | ||||||
|
seanbudd marked this conversation as resolved.
Outdated
|
||||||
| * Naming conventions: | ||||||
|
SaschaCowley marked this conversation as resolved.
|
||||||
| * functions/variables: `lowerCamelCase` | ||||||
| * classes: `UpperCamelCase` | ||||||
| * constants: `UPPER_SNAKE_CASE` | ||||||
| * scripts: `script_*` | ||||||
| * events: `event_*` | ||||||
|
seanbudd marked this conversation as resolved.
|
||||||
| * User-visible strings must be translatable via `_()` and include an appropriate translators comment. | ||||||
|
seanbudd marked this conversation as resolved.
Outdated
|
||||||
| * All public functions, classes, and methods must have Sphinx-style docstrings (without type declarations in docstrings). | ||||||
| Most internal functions, classes, and methods should also have docstrings, except where their purpose is clear from their name or code. | ||||||
| * Flag unnecessary import-time side effects and new module-level globals where avoidable. | ||||||
| * Prefer inclusive language and avoid terms disallowed by NVDA guidelines. | ||||||
|
seanbudd marked this conversation as resolved.
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. Shouldn't "NVDA guidelines" be replaced with the path to the file containing them, rather than assuming it will naturally know which guidelines apply in this situation?
Member
Author
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. see discussion here: #19738 (comment)
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. Ah yes, I recall that one, though didn't realize the context. All that aside, I'm seriously asking what makes us think it's going to read those referenced files at all? Not sure the whole "incorporated by reference" thing works here.
No idea if they mean web links or things it recognizes as file paths, but I've not seen any clarity that would suggest that file links in the repo work any better than web links. |
||||||
|
|
||||||
| ## PR checklist expectations | ||||||
|
|
||||||
| Ensure the PR content supports NVDA’s review checklist (`.github/PULL_REQUEST_TEMPLATE.md`): | ||||||
|
|
||||||
| * Documentation impact considered: | ||||||
| * changelog entry when required (`user_docs/en/changes.md`) | ||||||
|
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. Is it smart enough to know when to incorporate an external file as instructions (of which it can only take 1,000 lines before it gets confused), or the same format being used just to reference a file that it shouldn't read?
There has to be some way to confirm this, apart from the ambiguous statement in that blog.
Suggested change
No idea if that would make a difference, but at least it's a different format than the "incorporate by reference" files. I'm also concerned that "when required" is ambiguous, especially if it doesn't read in external files as I believe. |
||||||
| * user/developer docs updates where needed | ||||||
| * context-sensitive help for GUI options | ||||||
| * Testing strategy is clear and reproducible: | ||||||
| * unit/system/manual coverage discussed | ||||||
| * manual steps are concrete | ||||||
| * UX impact considered for: | ||||||
| * speech, braille, low vision | ||||||
| * browser differences where applicable | ||||||
| * localization implications | ||||||
| * API compatibility with add-ons is preserved unless explicitly intended and documented. | ||||||
|
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
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. The reason I commented on this before, is that the grammar as used, literally means:
It is supposed to mean that the compatibility should be preserved, unless incompatibility is intended (how does it know?) and documented. But it doesn't say that. It never indicates what the alternative to API compatibility is, which should probably lead the system to make the second subject agree with the first, which produces a very weird instruction. However this is phrased, I would drop the word "intended". The AI has no idea if it was intended. Documentation is the only deciding factor. |
||||||
|
|
||||||
| ## Security-specific review checks | ||||||
|
|
||||||
| For code that can expose sensitive information or run in privileged contexts: | ||||||
|
|
||||||
| * Check lock-screen object handling safeguards (for example, secure object filtering). | ||||||
| * Check secure-mode behavior (`globalVars.appArgs.secure`) and that blocked actions fail gracefully. | ||||||
| * Ensure changes do not leak information on secure screens. | ||||||
| * Check writable behavior (`NVDAState.shouldWriteToDisk`) and do not write to disk if running securely or if running from the launcher. | ||||||
|
seanbudd marked this conversation as resolved.
Outdated
|
||||||
|
|
||||||
|
seanbudd marked this conversation as resolved.
|
||||||
| ## Comment style for Copilot reviews | ||||||
|
|
||||||
| * Focus comments on changed lines and clear user/developer impact. | ||||||
| * Explain **why** something is a problem and **what** acceptable fix looks like. | ||||||
|
seanbudd marked this conversation as resolved.
Outdated
|
||||||
| * If uncertain, ask a precise question instead of making a weak assertion. | ||||||
| * Acknowledge valid trade-offs; do not insist on preferences when standards are satisfied. | ||||||
Uh oh!
There was an error while loading. Please reload this page.