Skip to content

revert: undo external PRs #34 and #35 for quality review #40

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.

Sign up for GitHub

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
Nelson Spence (Fieldnote-Echo) merged 1 commit into from
Mar 12, 2026
Merged

revert: undo external PRs #34 and #35 for quality review #40

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 6 additions & 11 deletions src/navi_bootstrap/cli.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,9 +53,8 @@ def cli() -> None:
"--spec",
Copy link
Copy Markdown
Contributor

@github-actions github-actions bot Mar 12, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔵 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.

required=True,
type=click.Path(exists=True, path_type=Path),
help="Path to the project spec YAML file",
)
@click.option("--pack", type=str, default=None, help="Name of the template pack to validate")
@click.option("--pack", type=str, default=None)
def validate(spec: Path, pack: str | None) -> None:
"""Validate a spec (and optionally a pack manifest)."""
try:
Expand All @@ -81,16 +80,14 @@ def validate(spec: Path, pack: str | None) -> None:
"--spec",
required=True,
type=click.Path(exists=True, path_type=Path),
help="Path to the project spec YAML file",
)
@click.option("--pack", required=True, type=str, help="Name of the template pack to render")
@click.option("--pack", required=True, type=str)
@click.option(
"--out",
type=click.Path(path_type=Path),
default=None,
help="Output directory (defaults to spec name)",
)
@click.option("--dry-run", is_flag=True, default=False, help="Preview output without writing files")
@click.option("--dry-run", is_flag=True, default=False)
@click.option("--skip-resolve", is_flag=True, default=False, help="Skip SHA resolution (offline)")
@click.option("--trust", is_flag=True, default=False, help="Execute hooks from manifest (unsafe)")
def render_cmd(
Expand Down Expand Up @@ -186,13 +183,12 @@ def render_cmd(
"--spec",
required=True,
type=click.Path(exists=True, path_type=Path),
help="Path to the project spec YAML file",
)
@click.option("--pack", required=True, type=str, help="Name of the template pack to apply")
@click.option("--pack", required=True, type=str)
@click.option(
"--target", required=True, type=click.Path(exists=True, file_okay=False, path_type=Path)
)
@click.option("--dry-run", is_flag=True, default=False, help="Preview output without writing files")
@click.option("--dry-run", is_flag=True, default=False)
@click.option("--skip-resolve", is_flag=True, default=False, help="Skip SHA resolution (offline)")
@click.option(
"--trust", is_flag=True, default=False, help="Execute hooks/validations from manifest (unsafe)"
Expand Down Expand Up @@ -295,9 +291,8 @@ def apply(
"--spec",
required=True,
type=click.Path(exists=True, path_type=Path),
help="Path to the project spec YAML file",
)
@click.option("--pack", required=True, type=str, help="Name of the template pack to diff")
@click.option("--pack", required=True, type=str)
@click.option(
"--target", required=True, type=click.Path(exists=True, file_okay=False, path_type=Path)
)
Expand Down
2 changes: 1 addition & 1 deletion src/navi_bootstrap/init.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -249,7 +249,7 @@ def detect_test_info(target: Path) -> dict[str, Any]:
content = test_file.read_text(errors="replace")
Copy link
Copy Markdown
Contributor

@github-actions github-actions bot Mar 12, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 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.

except (PermissionError, OSError):
continue
count += len(re.findall(r"^\s*(?:async\s+)?def test_", content, re.MULTILINE))
count += len(re.findall(r"^\s*def test_", content, re.MULTILINE))

return {
"test_framework": "pytest",
Expand Down
8 changes: 0 additions & 8 deletions tests/test_init.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -449,14 +449,6 @@ def test_ignores_non_test_files(self, tmp_path: Path) -> None:
result = detect_test_info(tmp_path)
Copy link
Copy Markdown
Contributor

@github-actions github-actions bot Mar 12, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔵 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.

assert result["test_count"] == 1

def test_detects_async_test_functions(self, tmp_path: Path) -> None:
tests = tmp_path / "tests"
tests.mkdir()
test_file = tests / "test_example.py"
test_file.write_text("def test_sync(): pass\nasync def test_async(): pass\n")
result = detect_test_info(tmp_path)
assert result["test_count"] == 2


# ---------------------------------------------------------------------------
# TestInspectProject
Expand Down
Loading