Adds PR review skill and checklist for pull request evaluations#7227
Adds PR review skill and checklist for pull request evaluations#7227waldekmastykarz wants to merge 4 commits into
Conversation
Co-authored-by: Copilot <[email protected]>
|
Isn't this simular to #7193? |
|
Yes it is. @milanholemans and I crossed streams. Like we agreed, we closed his PR in favor of this one. |
milanholemans
left a comment
milanholemans
left a comment
There was a problem hiding this comment.
Looks promising, would like to see this in action. Made a few remarks while reviewing the skill.
| - Escape user input in XML and URLs. | ||
| - Verbose and debug outputs are logged to stdErr (`logger.logToStderr` instead of `logger.log`). | ||
| - Do not do conditional output in JSON output mode; use `defaultProperties` for defining default properties. | ||
| - For commands with multiple options where the user is required to choose one, define these options using an `optionSet` without specific validation logic in the command's validate method. |
There was a problem hiding this comment.
Option sets don't exist anymore in Zod, let's remove it and let's also remove it from the docs.
|
|
||
| Check test files (`.spec.ts`) for: | ||
|
|
||
| - Coverage of happy path and error cases. |
There was a problem hiding this comment.
But also command name, description not being null, schema validation.
| - Coverage of happy path and error cases. | ||
| - Proper use of mocks and assertions. | ||
| - Tests that actually verify behavior (not just that code runs). | ||
|
|
There was a problem hiding this comment.
Let's add:
- Use
sinon.restore()inafter()to reset stubs/spies.
|
|
||
| Walk through each applicable item in the checklist and verify it against the diff. Key items to check: | ||
|
|
||
| - **Single quotes** for strings (not double quotes). |
There was a problem hiding this comment.
I try to maintain this for command files, but to be honest, mostly for test files, it's a hopeless task. People copy responses from API requests that contain double quotes, and it feels stupid to ask them to replace the entire response with single quotes.
There was a problem hiding this comment.
I think we should still aim for some level of consistency and AI should help us to enforce that. I suggest we keep this and remove if it becomes an issue.
Co-authored-by: Milan Holemans <[email protected]>
Co-authored-by: Milan Holemans <[email protected]>
- Remove obsolete PII/telemetry tracking rule (handled centrally by Zod) - Replace 'All tests must pass' with 'npm test must pass without errors' - Add Documentation Quality section (D) to SKILL.md - Add Reference Commands section pointing to example implementations Co-authored-by: Copilot <[email protected]>
3 participants
No description provided.