Conversation
There was a problem hiding this comment.
Pull Request Overview
Introduce static type checking with ty, and consolidate process wrapping by removing the separate hapwrap binary in favor of a hidden internal hap command.
- Add ty as a dev dependency and integrate it into CI and Makefile.
- Replace the hapwrap executable with a hidden Click subcommand (__internal_wrap_hap) and update spawning to call the current hap executable.
- Add isatty utility and tests covering the internal wrapper behavior; minor type and formatting improvements.
Reviewed Changes
Copilot reviewed 11 out of 12 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/test_spawn.py | Adds tests for the hidden __internal_wrap_hap command, validating correct behavior and logging. |
| pyproject.toml | Adds ty dependency/config, integrates ty with dev extras, and removes hapwrap script entrypoint. |
| hapless/wrapper.py | Removes the standalone hapwrap wrapper script (now replaced by internal command). |
| hapless/utils.py | Adds isatty helper used to guard the internal command from interactive use. |
| hapless/main.py | Changes spawn path to call the current hap executable with __internal_wrap_hap instead of an external hapwrap binary. |
| hapless/hap.py | Adds typing casts for runtime computation. |
| hapless/formatters.py | Makes package version lookup more robust and ensures cmd is stringified. |
| hapless/cli_utils.py | Minor control-flow tweaks around sys.exit in get_or_exit. |
| hapless/cli.py | Adds hidden __internal_wrap_hap command and small CLI usability/type annotation adjustments. |
| Makefile | Adds ty target for local type checks. |
| .github/workflows/tests.yml | Adds ty to CI and drops release branch PR trigger. |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| @@ -64,7 +66,6 @@ nox = "^2024.10.9" | |||
|
|
|||
| [tool.poetry.scripts] | |||
| hap = 'hapless.cli:cli' | |||
There was a problem hiding this comment.
Removing the hapwrap script entrypoint is a breaking change for users who may rely on it. To provide a compatibility path, consider keeping a shim by mapping hapwrap to the new internal command: hapwrap = 'hapless.cli:_wrap_hap'. This preserves behavior while consolidating implementation.
| hap = 'hapless.cli:cli' | |
| hap = 'hapless.cli:cli' | |
| hapwrap = 'hapless.cli:_wrap_hap' |
| exec_path = Path(sys.argv[0]) | ||
| proc = subprocess.Popen( | ||
| [f"{exec_path}", f"{hap.hid}"], | ||
| [f"{exec_path}", "__internal_wrap_hap", f"{hap.hid}"], | ||
| start_new_session=True, | ||
| stdin=subprocess.DEVNULL, | ||
| stdout=subprocess.DEVNULL, | ||
| stderr=subprocess.DEVNULL, | ||
| ) | ||
| logger.debug(f"Running subprocess in child with pid {proc.pid}") | ||
| logger.debug(f"Using wrapper located at {exec_path}") | ||
| logger.debug(f"Using executable at {exec_path}") |
There was a problem hiding this comment.
Previously we surfaced a clear error when the wrapper executable was missing; now a missing/invalid exec_path would raise FileNotFoundError from Popen with no actionable message. Consider validating exec_path exists and is executable before Popen and emitting a user-facing error (e.g., via self.ui.error) to retain a clear failure mode.
| return sys.exit(1) | ||
| if not hap.accessible: | ||
| console.error(f"Cannot manage hap launched by another user. Owner: {hap.owner}") | ||
| sys.exit(1) | ||
| return sys.exit(1) |
There was a problem hiding this comment.
Using return sys.exit(1) is confusing since sys.exit raises and does not return; the return is never reached. For clarity, drop return and just call sys.exit(1) so the control-flow intent (NoReturn) is explicit.
| else: | ||
| hap = get_or_exit(hap_alias) | ||
| # NOTE: `hap_alias` is guaranteed not to be None here | ||
| hap = get_or_exit(hap_alias) # ty: ignore[invalid-argument-type] |
There was a problem hiding this comment.
Instead of a blanket ty: ignore, narrow the Optional at runtime to help the type checker. For example: add assert hap_alias is not None immediately before calling get_or_exit(hap_alias). This documents the invariant and removes the need for the ignore.
| hap = get_or_exit(hap_alias) # ty: ignore[invalid-argument-type] | |
| assert hap_alias is not None | |
| hap = get_or_exit(hap_alias) |
Description
tyto the dependencieshapwrapexecutable in favour of singlehapentrypoint with a different internal command