generated from amazon-archives/__template_Apache-2.0
-
Notifications
You must be signed in to change notification settings - Fork 567
Port PR guidelines from sdk-typescript #1373
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
Merged
+219
−1
Merged
Changes from all commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,201 @@ | ||
| # Pull Request Description Guidelines | ||
|
|
||
| Good PR descriptions help reviewers understand the context and impact of your changes. They enable faster reviews, better decision-making, and serve as valuable historical documentation. | ||
|
|
||
| When creating a PR, follow the [GitHub PR template](../.github/PULL_REQUEST_TEMPLATE.md) and use these guidelines to fill it out effectively. | ||
|
|
||
| ## Who's Reading Your PR? | ||
|
|
||
| Write for senior engineers familiar with the SDK. Assume your reader: | ||
|
|
||
| - Understands the SDK's architecture and patterns | ||
| - Has context about the broader system | ||
| - Can read code diffs to understand implementation details | ||
| - Values concise, focused communication | ||
|
|
||
| ## What to Include | ||
|
|
||
| Every PR description should have: | ||
|
|
||
| 1. **Motivation** — Why is this change needed? | ||
| 2. **Public API Changes** — What changes to the public API (with code snippets)? | ||
| 3. **Use Cases** (optional) — When would developers use this feature? Only include for non-obvious functionality; skip for trivial changes or obvious fixes. | ||
| 4. **Breaking Changes** (if applicable) — What breaks and how to migrate? | ||
|
|
||
| ## Writing Principles | ||
|
|
||
| **Focus on WHY, not HOW:** | ||
|
|
||
| - ✅ "Hook providers need access to the agent's result to perform post-invocation actions like logging or analytics" | ||
| - ❌ "Added result field to AfterInvocationEvent dataclass" | ||
|
|
||
| **Document public API changes with example code snippets:** | ||
|
|
||
| - ✅ Show before/after code snippets for API changes | ||
| - ❌ List every file or line changed | ||
|
|
||
| **Be concise:** | ||
|
|
||
| - ✅ Use prose over bullet lists when possible | ||
| - ❌ Create exhaustive implementation checklists | ||
|
|
||
| **Emphasize user impact:** | ||
|
|
||
| - ✅ "Enables hooks to log conversation outcomes or trigger follow-up actions based on the result" | ||
| - ❌ "Updated AfterInvocationEvent to include optional AgentResult field" | ||
|
|
||
| ## What to Skip | ||
|
|
||
| Leave these out of your PR description: | ||
|
|
||
| - **Implementation details** — Code comments and commit messages cover this | ||
| - **Test coverage notes** — CI will catch issues; assume tests are comprehensive | ||
| - **Line-by-line change lists** — The diff provides this | ||
| - **Build/lint/coverage status** — CI handles verification | ||
| - **Commit hashes** — GitHub links commits automatically | ||
|
|
||
| ## Anti-patterns | ||
|
|
||
| ❌ **Over-detailed checklists:** | ||
|
|
||
| ```markdown | ||
| ### Type Definition Updates | ||
|
|
||
| - Added result field to AfterInvocationEvent dataclass | ||
| - Updated Agent._run_loop to capture and pass AgentResult | ||
| ``` | ||
|
|
||
| ❌ **Implementation notes reviewers don't need:** | ||
|
|
||
| ```markdown | ||
| ## Implementation Notes | ||
|
|
||
| - Result field defaults to None | ||
| - AgentResult is captured from EventLoopStopEvent before invoking hooks | ||
| ``` | ||
|
|
||
| ❌ **Test coverage bullets:** | ||
|
|
||
| ```markdown | ||
| ### Test Coverage | ||
|
|
||
| - Added test: AfterInvocationEvent includes AgentResult | ||
| - Added test: result is None when structured_output is used | ||
| ``` | ||
|
|
||
| ## Good Examples | ||
|
|
||
| ✅ **Motivation section:** | ||
|
|
||
| ```markdown | ||
| ## Motivation | ||
|
|
||
| Hook providers often need to perform actions based on the outcome of an agent's | ||
| invocation, such as logging results, updating metrics, or triggering follow-up | ||
| workflows. Currently, the `AfterInvocationEvent` doesn't provide access to the | ||
| `AgentResult`, forcing hook implementations to track state externally or miss | ||
| this information entirely. | ||
| ``` | ||
|
|
||
| ✅ **Public API Changes section:** | ||
|
|
||
| ````markdown | ||
| ## Public API Changes | ||
|
|
||
| `AfterInvocationEvent` now includes an optional `result` attribute containing | ||
| the `AgentResult`: | ||
|
|
||
| ```python | ||
| # Before: no access to result | ||
| class MyHook(HookProvider): | ||
| def on_after_invocation(self, event: AfterInvocationEvent) -> None: | ||
| # Could only access event.agent, no result available | ||
| logger.info("Invocation completed") | ||
|
|
||
| # After: result available for inspection | ||
| class MyHook(HookProvider): | ||
| def on_after_invocation(self, event: AfterInvocationEvent) -> None: | ||
| if event.result: | ||
| logger.info(f"Completed with stop_reason: {event.result.stop_reason}") | ||
| ``` | ||
|
|
||
| The `result` field is `None` when invoked from `structured_output` methods. | ||
|
|
||
| ```` | ||
|
|
||
| ✅ **Use Cases section:** | ||
|
|
||
| ```markdown | ||
| ## Use Cases | ||
|
|
||
| - **Result logging**: Log conversation outcomes including stop reasons and token usage | ||
| - **Analytics**: Track agent performance metrics based on invocation results | ||
| - **Conditional workflows**: Trigger follow-up actions based on how the agent completed | ||
| ```` | ||
|
|
||
| ## Template | ||
|
|
||
| ````markdown | ||
| ## Motivation | ||
|
|
||
| [Explain WHY this change is needed. What problem does it solve? What limitation | ||
| does it address? What user need does it fulfill?] | ||
|
|
||
| Resolves: #[issue-number] | ||
|
|
||
| ## Public API Changes | ||
|
|
||
| [Document changes to public APIs with before/after code snippets. If no public | ||
| API changes, state "No public API changes."] | ||
|
|
||
| ```python | ||
| # Before | ||
| [existing API usage] | ||
|
|
||
| # After | ||
| [new API usage] | ||
| ``` | ||
|
|
||
| [Explain behavior, parameters, return values, and backward compatibility.] | ||
|
|
||
| ## Use Cases (optional) | ||
|
|
||
| [Only include for non-obvious functionality. Provide 1-3 concrete use cases | ||
| showing when developers would use this feature. Skip for trivial changes obvious fixes..] | ||
|
|
||
| ## Breaking Changes (if applicable) | ||
|
|
||
| [If this is a breaking change, explain what breaks and provide migration guidance.] | ||
|
|
||
| ### Migration | ||
|
|
||
| ```python | ||
| # Before | ||
| [old code] | ||
|
|
||
| # After | ||
| [new code] | ||
| ``` | ||
|
|
||
| ```` | ||
|
|
||
| ## Why These Guidelines? | ||
|
|
||
| **Focus on WHY over HOW** because code diffs show implementation details, commit messages document granular changes, and PR descriptions provide the broader context reviewers need. | ||
|
|
||
| **Skip test/lint/coverage details** because CI pipelines verify these automatically. Including them adds noise without value. | ||
|
|
||
| **Write for senior engineers** to enable concise, technical communication without redundant explanations. | ||
|
|
||
| ## References | ||
|
|
||
| - [Conventional Commits](https://www.conventionalcommits.org/) | ||
| - [Google's Code Review Guidelines](https://google.github.io/eng-practices/review/) | ||
|
|
||
| ## Checklist Items | ||
|
|
||
| - [ ] Does the PR description target a Senior Engineer familiar with the project? | ||
| - [ ] Does the PR description give an overview of the feature being implemented, including any notes on key implementation decisions | ||
| - [ ] Does the PR include a "Resolves #<ISSUE NUMBER>" in the body and is not bolded? | ||
| - [ ] Does the PR contain the motivation or use-cases behind the change? | ||
| - [ ] Does the PR omit irrelevant details not needed for historical reference? | ||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Uh oh!
There was an error while loading. Please reload this page.