docs: move SDK development guidance to local READMEs - #2253
Conversation
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Moves SDK-specific development guidance from the shared contribution guide into each SDK README.
Changes:
- Adds prerequisites and validation workflows for all six SDKs.
- Replaces duplicated contribution instructions with README links.
- Documents pinned Rust and Java toolchains.
Show a summary per file
| File | Description |
|---|---|
CONTRIBUTING.md |
Links to SDK-specific guidance. |
nodejs/README.md |
Adds Node.js workflow. |
python/README.md |
Adds Python workflow. |
go/README.md |
Adds Go workflow. |
dotnet/README.md |
Adds .NET workflow. |
rust/README.md |
Adds Rust toolchain and workflow. |
java/README.md |
Updates Java development workflow. |
Review details
Suppressed comments (5)
python/README.md:1161
- This local pytest command leaves
GITHUB_ACTIONSunset. The E2E teardown therefore permits snapshot writes (python/e2e/conftest.py:40-42), and the shared proxy can contact live CAPI when a snapshot does not match. Document the replay-only invocation for routine contributor validation.
nodejs/README.md:1126 - This local test command leaves
GITHUB_ACTIONSunset, so an E2E snapshot miss falls through to the live CAPI endpoint and successful runs can rewrite snapshot files (test/harness/replayingCapiProxy.ts:185-231,560-574). Make the documented contributor workflow replay-only to avoid network calls and fixture changes during routine validation.
dotnet/README.md:1054 - This local test invocation does not set
GITHUB_ACTIONS; consequentlyE2ETestContexttreats the run as record mode (dotnet/test/Harness/E2ETestContext.cs:476-478) and the replay proxy may contact live CAPI and write snapshots. The documented validation command should force replay-only mode.
go/README.md:995 test.shdoes not setGITHUB_ACTIONS, so this documented local workflow allows the shared replay proxy to call live CAPI on a snapshot miss and overwrite captures. Set replay-only mode explicitly for the contributor test run.
rust/README.md:985- Plain
cargo testdoes not exercise the Rust E2E suite becausetests/e2e.rsis gated bytest-support, and the two explicitly declared integration targets also require that feature (Cargo.toml:81-91). Enabling it also requires installing the CLI undernodejs/node_modules(tests/e2e/support.rs:1172-1203), which this workflow currently omits, so the documented test command gives a passing but incomplete validation. Install the Node runtime and run tests withtest-support.
- Files reviewed: 7/7 changed files
- Comments generated: 0
- Review effort level: Balanced
There was a problem hiding this comment.
Review details
Suppressed comments (6)
rust/README.md:975
- This command silently skips the Rust tests that are gated by
test-support:session_testandprotocol_version_testdeclare that required feature inCargo.toml, whilee2e.rsandjsonrpc_test.rsare entirely disabled without it. As written, contributors do not run the full SDK suite despite installing the E2E harness; enabletest-supportin the documented command.
cd rust && cargo test
dotnet/README.md:1043
- The test workflow invokes
npm ci, but Node.js is not listed as a development prerequisite. The harness enforces Node^20.19.0 || >=22.12.0, so the current prerequisites are insufficient for running this command.
Development requires [.NET SDK 10+](https://dotnet.microsoft.com/download). From the repository root:
rust/README.md:971
- The workflow below invokes
npm ci, but this README does not list Node.js as a development prerequisite. Document the harness's Node^20.19.0 || >=22.12.0requirement so these instructions work in a fresh environment.
From the repository root:
java/README.md:425
mvn verifyexecutesnpm cifor bothtest/harnessandnodejsthroughexec-maven-plugin, but Node.js is omitted from this prerequisite list. Add the harness's Node^20.19.0 || >=22.12.0requirement; otherwise the documented command fails in a JDK/Maven-only environment.
Development requires JDK 25+ and [Maven](https://maven.apache.org/download.cgi). From the repository root:
python/README.md:1148
- The documented workflow invokes
npm ci, but Node.js is not listed as a development prerequisite anywhere in this README. The harness requires Node^20.19.0 || >=22.12.0, so contributors with no Node installation—or an older one—cannot follow these instructions.
Install [uv](https://docs.astral.sh/uv/), then from the repository root:
go/README.md:983
- The test workflow below requires
npm, but this README only lists Go as the relevant development tool. Document the test harness's Node^20.19.0 || >=22.12.0requirement so a fresh contributor can run the command successfully.
From the repository root:
- Files reviewed: 7/7 changed files
- Comments generated: 0 new
- Review effort level: Balanced
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
Addressed the valid review feedback by linking Python, Go, .NET, Rust, and Java development setup to the authoritative Node.js prerequisite. The Rust |
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Review details
Suppressed comments (1)
CONTRIBUTING.md:38
- The linked development sections generally document setup and tests, not standalone build commands, and deleting the former “All SDKs” note also removes the only explicit Node.js prerequisite even though the Python, Go, .NET, and Rust workflows below all invoke
npm. Keep Node.js as a shared prerequisite here (linked to its maintained version source) and describe the delegated scope accurately.
Setup, build, and test instructions are maintained with each SDK:
- Files reviewed: 7/7 changed files
- Comments generated: 0 new
- Review effort level: Balanced
Cross-SDK Consistency Review ✅This PR modifies only documentation files (README.md files for all six SDKs and CONTRIBUTING.md). No SDK source code was changed. The documentation changes are applied consistently across all six SDK implementations:
No cross-SDK consistency issues found.
|
There was a problem hiding this comment.
Review details
Suppressed comments (1)
java/README.md:425
- The prerequisite list omits tools required by the workflow immediately below: the commands invoke
mvn, and the final validation phase requires a JDK 17 installation in addition to JDK 25. A contributor with only the listed prerequisites cannot complete the documented steps; list Maven and JDK 17 (or switch the commands to the checked-in Maven wrapper and make the JDK 17 phase explicitly optional).
Requires JDK 25 or later and a supported [Node.js version](../nodejs/README.md#prerequisites) for development. The following steps validate the artifact built with JDK 25 runs on both 25 and 17, preserving the MR-JAR behavior.
- Files reviewed: 7/7 changed files
- Comments generated: 0 new
- Review effort level: Balanced
* docs: move SDK development guidance to local READMEs Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * docs: simplify SDK development instructions Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * docs: address SDK development review feedback Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * docs: document Node requirement for SDK tests Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * docs: keep local test commands cross-platform Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --------- Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Summary
This is a smaller alternative to #2247 that avoids duplicating mutable language-specific instructions in
CONTRIBUTING.md.Validation
uv syncand ran its formatting, linting, and type checks