revert: undo external PRs #34 and #35 for quality review#40
revert: undo external PRs #34 and #35 for quality review#40Nelson Spence (Fieldnote-Echo) merged 1 commit intomainfrom
Conversation
There was a problem hiding this comment.
Pull request overview
This PR reverts two recently merged external changes to return the codebase to its prior behavior while the underlying improvements receive a quality review before being reintroduced.
Changes:
- Removes previously added Click
help=strings for several CLI options invalidate,render,apply, anddiff. - Reverts pytest test-function counting back to synchronous-only (
def test_...) detection. - Removes the test that asserted async test detection behavior.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
src/navi_bootstrap/cli.py |
Reverts added option help text to restore prior CLI help output. |
src/navi_bootstrap/init.py |
Reverts the regex used to count pytest tests back to sync-only detection. |
tests/test_init.py |
Removes the async test-counting unit test that no longer matches the reverted implementation. |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| @@ -249,7 +249,7 @@ def detect_test_info(target: Path) -> dict[str, Any]: | |||
| content = test_file.read_text(errors="replace") | |||
There was a problem hiding this comment.
🟡 MEDIUM: Async test detection reverted-potential test undercount
Confidence: 90%
The regular expression for counting test functions in detect_test_info was reverted from detecting both sync and async tests (r"^\s*(?:async\s+)?def test_") to only sync (r"^\s*def test_"). This will miss async test functions, underreporting test count if async test functions exist.
Suggestion: If async test functions should be counted, reinstate the previous regex: r"^\s*(?:async\s+)?def test_". If not, document why async test functions should be excluded.
— Reverting this means async test functions are no longer counted. If that's intentional, make sure everyone agrees. If not, expect confusion later.
| @@ -449,14 +449,6 @@ def test_ignores_non_test_files(self, tmp_path: Path) -> None: | |||
| result = detect_test_info(tmp_path) | |||
There was a problem hiding this comment.
🔵 LOW: Async test detection test removed
Confidence: 95%
The test case for detecting async test functions was removed. With the regex reverted to match only sync tests, async test coverage detection is no longer tested. This reduces confidence that the test counting function will be accurate if async test support is reintroduced.
Suggestion: If async test support is removed intentionally, consider documenting the decision and, if possible, revisiting the need for such tests before merging future related changes.
— The coverage for async test detection goes with the code. Makes sense, but if async detection returns, so should its test.
| @@ -53,9 +53,8 @@ def cli() -> None: | |||
| "--spec", | |||
There was a problem hiding this comment.
🔵 LOW: Loss of CLI help text
Confidence: 98%
Several help parameter strings from click options were removed as part of this revert. This reduces CLI discoverability and user experience for newcomers or users unfamiliar with argument usage.
Suggestion: When reintroducing these features, preserve detailed help text for all CLI options where possible. Consider adding a governance rule requiring help text on CLI arguments.
— It's a revert, so not a bug-but the CLI's self-documentation takes a hit. Just make sure these help texts come back.
✅ Grippy Review — PASSScore: 89/100 | Findings: 3 Delta: 3 new Commit: 0b5ddcb |
project-navi-bot
left a comment
project-navi-bot
left a comment
There was a problem hiding this comment.
All required CI checks passed. Auto-approved by navi-bot.
project-navi-bot
left a comment
project-navi-bot
left a comment
There was a problem hiding this comment.
All required CI checks passed. Auto-approved by navi-bot.
Summary
Issues #30 and #32 remain open for proper reimplementation.
Files changed
src/navi_bootstrap/cli.py— removed help text additionssrc/navi_bootstrap/init.py— reverted regex to sync-onlytests/test_init.py— removed async test function test