Add an AI review skill to guide users#168
Add an AI review skill to guide users#168ahuang11 wants to merge 5 commits intoMarcSkovMadsen:mainfrom
Conversation
Updated the title of the AI Review document to 'Review' and retained the purpose section.
There was a problem hiding this comment.
Pull request overview
Adds a new “review” skill intended to help users do a quick AI-assisted code review (pre-PR / pre-commit style) by outlining a short checklist of review heuristics.
Changes:
- Introduces
skills/review/SKILL.mdwith a purpose statement and review checklist.
skills/review/SKILL.md
Outdated
| - [ ] when the attribute is known to be present, do not use `hasattr` & `getattr` | ||
| - [ ] use try/except only on the clause that may raise the exception, not on the whole function | ||
| - [ ] try to pin the exception type in the except clause, do not use a bare except | ||
| - [ ] return or continue early to avoid deep nesting | ||
| - [ ] anything over 3 levels of nesting should be refactored into helper functions | ||
| - [ ] imports should be at the top of the file, not inside functions unless there is a specific reason to do so (slow import, circular import, etc.) | ||
| - [ ] understanding the full changes, is there a more efficient or cleaner way to achieve the same result? | ||
| - [ ] consistent and readable naming conventions, e.g. if FollowUpSuggestion is used, the variable name should be follow_up_suggestion, not followup_suggestion or follow_up_suggestions (plural) |
There was a problem hiding this comment.
Checklist items are written as lower-case sentences, which is inconsistent with the established style in other skills (they generally use "DO"/"DON'T" phrasing and sentence-case bullets). Consider rewriting these bullets to match the existing skills formatting for consistency and readability.
| - [ ] when the attribute is known to be present, do not use `hasattr` & `getattr` | |
| - [ ] use try/except only on the clause that may raise the exception, not on the whole function | |
| - [ ] try to pin the exception type in the except clause, do not use a bare except | |
| - [ ] return or continue early to avoid deep nesting | |
| - [ ] anything over 3 levels of nesting should be refactored into helper functions | |
| - [ ] imports should be at the top of the file, not inside functions unless there is a specific reason to do so (slow import, circular import, etc.) | |
| - [ ] understanding the full changes, is there a more efficient or cleaner way to achieve the same result? | |
| - [ ] consistent and readable naming conventions, e.g. if FollowUpSuggestion is used, the variable name should be follow_up_suggestion, not followup_suggestion or follow_up_suggestions (plural) | |
| - [ ] DO avoid using `hasattr` and `getattr` when the attribute is known to be present. | |
| - [ ] DO apply `try`/`except` only to the clause that may raise the exception, not to the whole function. | |
| - [ ] DO pin the exception type in the `except` clause; DON'T use a bare `except`. | |
| - [ ] DO return or continue early to avoid deep nesting. | |
| - [ ] DO refactor code with more than three levels of nesting into helper functions. | |
| - [ ] DO place imports at the top of the file; DON'T place imports inside functions unless there is a specific reason (slow import, circular import, etc.). | |
| - [ ] DO consider the full set of changes and whether there is a more efficient or cleaner way to achieve the same result. | |
| - [ ] DO use consistent and readable naming conventions, e.g., if `FollowUpSuggestion` is used, the variable name should be `follow_up_suggestion`, not `followup_suggestion` or `follow_up_suggestions` (plural). |
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Andrew <15331990+ahuang11@users.noreply.github.com>
|
My overall comment here is that this is definitely useful but I'm not sure it belongs here. |
After a user finishes prompting the LLM, prior to creating a PR, they should use this skill to do a quick review, like pre-commit.