Skip to content

chore(claude): learn from #278#284

Closed
claude[bot] wants to merge 1 commit into
mainfrom
claude/learn-from-278
Closed

chore(claude): learn from #278#284
claude[bot] wants to merge 1 commit into
mainfrom
claude/learn-from-278

Conversation

@claude
Copy link
Copy Markdown
Contributor

@claude claude Bot commented May 7, 2026

What this does

Captures one rule from the reviewer feedback on #278 and adds it to
CLAUDE.md as hard rule #6:

Tests that mutate module-level state must save and restore it via
try/finally.

Context: #278 added a second module-level dedup flag
(_SKIP_TIMESTAMP_WARNED) alongside the existing
_CONTROL_MODE_WARNED in src/opentau/datasets/lerobot_dataset.py.
The first round of that PR's test mutated the flag without restoring
it; the reviewer's @claude fix the nit comment asked for a
try/finally save/restore so the test doesn't leave the flag
flipped for subsequent tests in the same pytest-xdist worker. The
shape of the fix landed as test_skip_timestamp_warning_emitted_once_per_process
in tests/datasets/test_datasets.py, which the new rule points
to as the canonical example.

This is a recurring concern — there are now two such flags in the
same file, and the dedup pattern is likely to grow as more
mixture-wide warnings get added. Recording it here means future
tests in this codebase get the convention right on the first try
without re-litigating the nit.

No code changes; documentation only.

How it was tested

  • N/A — single-line addition to CLAUDE.md, no executable code touched.

How to checkout & try? (for the reviewer)

git fetch origin claude/learn-from-278
git checkout claude/learn-from-278
git diff main -- CLAUDE.md

Checklist

  • I have added Google-style docstrings to important functions and ensured function parameters are typed.
  • My PR includes policy-related changes.
    • If the above is checked: I have run the GPU pytests (pytest -m "gpu") and regression tests.

🤖 Generated with Claude Code

Add a hard rule that tests mutating module-level state (e.g. the
dedup flags `_CONTROL_MODE_WARNED` / `_SKIP_TIMESTAMP_WARNED` in
`lerobot_dataset.py`) must save and restore the original value
via try/finally. Captured from the reviewer "fix the nit"
feedback on #278.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@claude
Copy link
Copy Markdown
Contributor Author

claude Bot commented May 7, 2026

[claude-review] summary for commit 65f6b63

No blocking issues found.

  • nitCLAUDE.md:35 — Companion test test_control_mode_warning_emitted_once_per_repo (tests/datasets/test_datasets.py:1010) calls _CONTROL_MODE_WARNED.discard(test_repo) before the assertion but has no try/finally to restore prior membership; under the new rule it should follow the same save/restore shape. Out of scope for this docs-only PR, but a natural follow-up since the rule explicitly cites _CONTROL_MODE_WARNED as one of the flags it covers.

@shuheng-liu
Copy link
Copy Markdown
Member

Not worth an entry in project-level CLAUDE.md

@shuheng-liu shuheng-liu closed this May 7, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant