-
Notifications
You must be signed in to change notification settings - Fork 31
Guide agents(s) to be more concise on issue & PR descriptions #338
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
|
|
||
| ### Pull Request Best Practices | ||
|
|
||
| - You MUST follow the PR description guidelines in [docs/PR.md](../../docs/PR.md) |
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.
nit: must or should? we have the same item twice with different wording
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.
Good catch; switched to MUST above
docs/PR.md
Outdated
|
|
||
| **Document public API changes with examples:** | ||
|
|
||
| - ✅ Show before/after code examples for API changes |
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.
nit: code snippets? I generally tend to tell it to abstract out the unrelated code, so we dont have bunch of scaffolding in PR description
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.
It seemed pretty good at doing snippets regardless, but switched to snippets because why not :)
Updated language for clarity and consistency in PR guidelines.
| Before creating or updating a PR, you MUST copy the checklist from [docs/PR.md](../../docs/PR.md) into your progress notes and explicitly verify each item. For each checklist item, you MUST: | ||
|
|
||
| 1. Copy the checklist item verbatim | ||
| 2. Mark it as `[x]` (pass) or `[ ]` (fail) |
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.
nit: consider - for the failure case
| - You MUST check that all tasks are complete before proceeding | ||
| - You MUST reference your notes for the issue you are creating a pull request for | ||
| - You MUST NOT commit changes until builds AND tests have been verified because committing broken code can disrupt the development workflow and introduce bugs into the codebase | ||
| - You MUST NOT commit changes until builds AND tests have been verified because committing broken code can disrupt the development workflow and introduce bugs into the codebase |
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.
because committing broken code can disrupt the development workflow and introduce bugs into the codebase
Don't think this is necessary as we don't provide a justification for the other items here. We just command the agent to follow
Description
Update agent guidance to include more useful Pull-Request guidance.
I've noticed our agents generate pretty bad descriptions in pull-requests, so I've been playing around with better guidance - specifically by:
The PR.md file can also be used by regular contributors for guidance on how to create PRs
I also had a small change here to stop including effort estimations in task creation, since I think it's unless noise.
Testing
How have you tested the change?
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.