Skip to content

fix: address pre-publish review issues#12

Merged
joeVenner merged 2 commits into
mainfrom
fix/pre-publish-review-issues
Apr 21, 2026
Merged

fix: address pre-publish review issues#12
joeVenner merged 2 commits into
mainfrom
fix/pre-publish-review-issues

Conversation

@joeVenner
Copy link
Copy Markdown
Contributor

Summary

Addresses three issues flagged in the pre-publish code review:

  1. Remove duplicate petfeeder pitfall lines (src/iotcli/skills/generator.py) — two conflicting lines (N=1–10 vs N=1–60) were present; kept the correct one.

  2. Skip MCP tests when optional dep is missing (tests/test_mcp.py) — added pytest.importorskip("mcp") so the test suite passes cleanly without the [mcp] extra installed.

  3. Move DeviceController after device check (src/iotcli/cli/commands/control.py) — avoids unnecessary object creation when the device name is a typo.

Test plan

  • pytest -q passes (4 passed, 1 skipped)
  • No hardcoded secrets or sensitive data introduced
  • All changes are local, reversible, and non-breaking

🤖 Generated with Claude Code

- Remove duplicate/conflicting petfeeder pitfall lines (1-10 vs 1-60)
- Skip MCP tests gracefully when optional mcp dependency is missing
- Move DeviceController instantiation after device existence check
@github-actions github-actions Bot added size: XS < 10 lines changed bug Something isn't working area: cli CLI commands area: skills AI agent skill generator tests Test changes labels Apr 21, 2026
@greptile-apps
Copy link
Copy Markdown
Contributor

greptile-apps Bot commented Apr 21, 2026

Greptile Summary

This PR addresses three pre-publish review issues across the CLI command layer, skill generator, and MCP test suite — all changes are small, targeted, and non-breaking.

  • control.py: DeviceController instantiation is moved to after the device-not-found guard, avoiding unnecessary object creation on bad device names. The guard still lacks an explicit return after out.error(), leaving control flow implicitly reliant on sys.exit() being called inside that method.
  • generator.py: Removes a duplicate, conflicting pitfall bullet for the petfeeder profile (N = 1–60 vs N = 1–10); the correct N = 1–10 entry is retained.
  • test_mcp.py: Adds pytest.importorskip(\"mcp\") at module level — the standard pytest idiom to skip the entire module when the optional [mcp] extra is absent — so pytest -q passes cleanly without the extra installed.

Confidence Score: 5/5

Safe to merge — all three changes are small and correct; the one style note is non-blocking.

The three fixes are all well-scoped: the duplicate pitfall line removal is straightforward, the importorskip pattern is idiomatic pytest, and the DeviceController reordering is logically correct. The only finding is a missing explicit return after out.error(), which is a pre-existing style gap that doesn't affect current behaviour since out.error() unconditionally calls sys.exit().

No files require special attention; the mild guard note in control.py is a follow-up cleanup, not a blocker.

Important Files Changed

Filename Overview
src/iotcli/cli/commands/control.py Moves DeviceController instantiation after the device-not-found guard — correct intent, but the guard still lacks an explicit return, leaving the flow implicitly dependent on out.error() always calling sys.exit().
src/iotcli/skills/generator.py Removes the conflicting duplicate petfeeder pitfall bullet (N = 1–60); the correct N = 1–10 bullet is retained.
tests/test_mcp.py Adds pytest.importorskip("mcp") at module level — standard pytest idiom that correctly skips the entire module when the optional [mcp] extra is not installed.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A[control command invoked] --> B[resolve device via cfg.get_device_or_none]
    B --> C{device found?}
    C -- No --> D[out.error — sys.exit]
    C -- Yes --> E[create DeviceController]
    E --> F{action?}
    F -- on --> G[ctrl.turn_on]
    F -- off --> H[ctrl.turn_off]
    F -- status --> I[ctrl.get_status]
    F -- set --> J[ctrl.set_value]
    G & H & I & J --> K[update config status / output result]
Loading

Reviews (1): Last reviewed commit: "fix(skills,cli,tests): address pre-publi..." | Re-trigger Greptile

Comment thread src/iotcli/cli/commands/control.py Outdated
Co-authored-by: greptile-apps[bot] <165735046+greptile-apps[bot]@users.noreply.github.com>
@github-actions github-actions Bot added tests Test changes and removed tests Test changes labels Apr 21, 2026
@joeVenner joeVenner merged commit 536217a into main Apr 21, 2026
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area: cli CLI commands area: skills AI agent skill generator bug Something isn't working size: XS < 10 lines changed tests Test changes

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant