-
Notifications
You must be signed in to change notification settings - Fork 3.4k
Update Copilot instructions based on PR feedback analysis #37636
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
Conversation
Co-authored-by: AndriySvyryd <6539701+AndriySvyryd@users.noreply.github.com>
Co-authored-by: AndriySvyryd <6539701+AndriySvyryd@users.noreply.github.com>
AndriySvyryd
left a comment
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.
…nalysis to Implementation Guidelines Co-authored-by: AndriySvyryd <6539701+AndriySvyryd@users.noreply.github.com>
|
Examined older Copilot PRs (#37495, #37456, #37437, #37397, #37394, #37392, #37389, #37387, #37302, #37303, #37287, #37101). The main patterns found reinforce existing instructions - removing unnecessary comments, using |
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.
Pull request overview
This PR updates the GitHub Copilot instructions file based on feedback analysis from previous Copilot-submitted PRs. The goal is to improve future Copilot PR quality by addressing common issues and patterns identified through systematic review of PRs #37495, #37456, #37437, and many others going back to #36648.
Changes:
- Enhanced comment guidelines to explicitly emphasize minimal comments and self-explanatory code
- Clarified pattern matching guidelines with concrete examples
- Added cross-platform testing and dotnet-ef tool testing guidelines
- Added new "Dependency and Version Management" section with proper version property usage
- Added implementation guideline to review surrounding code for similar patterns
- Added comprehensive "Pull Request Guidelines" section with servicing PR template
Co-authored-by: AndriySvyryd <6539701+AndriySvyryd@users.noreply.github.com>
roji
left a comment
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.
@AndriySvyryd it's important to keep in mind that too much instructions is also a a problem, as it eats up the context window. I'm not sure what a good balance here is - we'll figure this out as we go along.
…ideline Co-authored-by: AndriySvyryd <6539701+AndriySvyryd@users.noreply.github.com>
Changes Made
Based on reviewer feedback:
release/*orfeature/*branches require special permission" lineAdditional Analysis (PRs older than #37518)
Examined PRs: #37495, #37456, #37437, #37397, #37394, #37392, #37389, #37387, #37302, #37303, #37287, #37101
Key patterns found that reinforce existing instructions:
varfor local variablesis null/is not nullpattern matchingusingblocks with assertions inside in testsAdditional Analysis (PRs older than #37101)
Examined PRs: #37056, #37054, #37052, #37020, #37019, #36986, #36772, #36771, #36751, #36750, #36717, #36671, #36670, #36669, #36668, #36667, #36666, #36665, #36651, #36650, #36649, #36648
Key patterns found that reinforce existing instructions:
Locktype (PR [rc2] Fix concurrency issue in RelationalConnection ambient transaction handling #36665)These patterns are already covered by existing instructions (minimal comments, pattern matching, following existing test patterns, not adding unnecessary code). No additional instructions needed.
Original prompt
✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.