docs(test-support): lead with the closure form most tables use#2707
Conversation
|
Auto-sync is disabled for draft pull requests in this repository. Workflows must be run manually. Contributors can view more details about this message here. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (1)
✅ Files skipped from review due to trivial changes (1)
Summary by CodeRabbit
WalkthroughDocumentation in ChangesMacro Documentation:
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~3 minutes Possibly related issues
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
@coderabbitai PTAL, thanks! |
🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
gh pr view 2707 --repo NVIDIA/infra-controller --json title,body,files,additions,deletions
echo "=== DIFF ==="
gh pr diff 2707 --repo NVIDIA/infra-controllerLength of output: 4204
One minor editorial nit worth addressing before merge:
// current
//! For a bare named function the `fn:` shorthand reads a little cleaner --
// suggested
//! For a bare named function, the `fn:` shorthand reads a little cleaner --
Everything else looks solid:
One question on the "Several inputs per row" doctest: does Otherwise, this is a clean, focused improvement. Happy to approve once the comma nit is addressed (or if you prefer to leave it as-is, that's a reasonable call too). ✅ Action performedReview finished.
|
The module docs taught the `fn_path:` shorthand first, but almost every table in the tree reaches for the `run =` closure form instead -- 710 of 734 macro calls use it -- and the closing guidance pointed at a local `struct Case` plus `assert_outcome` pattern that nothing in the tree actually uses. A reader learned the rare form and met the common one only by reading other tests. The docs now lead with `run = |...|`, keep the `fn:` shorthand as the aside it should be, document the real several-inputs idiom (a local struct used as the row `input` and destructured in the closure), and scope `assert_outcome` to the hand-written-case escape hatch it is. Docs only -- the new examples run as doctests. Signed-off-by: Chet Nichols III <chetn@nvidia.com>
|
@coderabbitai Thanks for the eyes. Updated -- added the comma after |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
# List CodeRabbit's review comments on PR `#2707` to find the comment ID to resolve
gh api graphql \
-F owner="NVIDIA" \
-F repo="infra-controller" \
-F pr=2707 \
-f query='
query($owner: String!, $repo: String!, $pr: Int!) {
repository(owner: $owner, name: $repo) {
pullRequest(number: $pr) {
reviewThreads(first: 20) {
nodes {
id
isResolved
comments(first: 1) {
nodes {
author { login }
body
}
}
}
}
}
}
}'Length of output: 692
(ʘ‿ʘ)ノ🐇 |
🔍 Container Scan Summary
Per-CVE detail lives in the per-service |
2 participants
Wave 1 of the carbide-test-support integration (#2692).
The module docs led with the
fn_path:shorthand, but 710 of 734 macro calls use therun =closure form instead, and the closing guidance pointed at a localstruct Caseplusassert_outcomepattern that nothing in the tree uses. This leads the examples withrun = |...|, keeps thefn:shorthand as an aside, documents the real several-inputs idiom (a local struct used as the rowinputand destructured in the closure), and scopesassert_outcometo the hand-written-case escape hatch.Docs only -- the new examples run as doctests (
cargo test --doc -p carbide-test-support: 9 pass).Addresses #2693.