feat(cli): add py script type & Python interpreter resolution (#3278)#3285
Open
mnriem wants to merge 4 commits into
Open
feat(cli): add py script type & Python interpreter resolution (#3278)#3285mnriem wants to merge 4 commits into
py script type & Python interpreter resolution (#3278)#3285mnriem wants to merge 4 commits into
Conversation
…ub#3278) Introduce a third script variant alongside `sh`/`ps` as the foundation for unifying workflow scripts under a single Python implementation. - Add `"py": "Python"` to `SCRIPT_TYPE_CHOICES`; `VALID_SCRIPT_TYPES` consumers (init workflow step, init command, _helpers) pick it up automatically since they derive from that mapping. - Add `IntegrationBase.resolve_python_interpreter()` (project venv → `python3` → `python`, falling back to `python3`). - Prefix the resolved interpreter when `process_template()` expands `{SCRIPT}` for the `py` script type so `.py` scripts run portably (notably on Windows); thread `project_root` through callers so venv preference works. - Make `install_scripts()` mark copied `.py` files executable too. Includes positive and negative unit tests for interpreter resolution, `py` template processing, the new choice, and script installation. Assisted-by: GitHub Copilot (model: Claude Opus 4.8, autonomous) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
Pull request overview
Adds first-class support for a Python (py) workflow script variant alongside existing sh/ps options, including template rendering support that prefixes Python scripts with a resolved interpreter for cross-platform execution.
Changes:
- Adds
"py": "Python"toSCRIPT_TYPE_CHOICESso allVALID_SCRIPT_TYPESconsumers acceptpy. - Updates
IntegrationBase.process_template()to prefix{SCRIPT}with a resolved Python interpreter whenscript_type == "py", and threadsproject_rootthrough integration setup call sites so venv preference can work. - Extends
install_scripts()to mark copied.pyscripts executable, and adds unit tests for interpreter resolution, template processing, and chmod behavior.
Show a summary per file
| File | Description |
|---|---|
| tests/test_commands_package.py | Verifies py is present in script type choices and workflow init valid script types. |
| tests/integrations/test_base.py | Adds unit coverage for interpreter resolution, py template rendering behavior, and .py chmod in install_scripts(). |
| src/specify_cli/_agent_config.py | Introduces py as a supported script type choice. |
| src/specify_cli/integrations/base.py | Implements Python interpreter resolution, py {SCRIPT} prefixing, threads project_root, and chmods .py scripts. |
| src/specify_cli/integrations/copilot/init.py | Passes project_root into template processing for venv-aware interpreter selection. |
| src/specify_cli/integrations/forge/init.py | Passes project_root into template processing for venv-aware interpreter selection. |
| src/specify_cli/integrations/generic/init.py | Passes project_root into template processing for venv-aware interpreter selection. |
| src/specify_cli/integrations/hermes/init.py | Passes project_root into template processing for venv-aware interpreter selection. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 8/8 changed files
- Comments generated: 2
- Review effort level: Low
Address PR review feedback on github#3285: - `resolve_python_interpreter()` now returns the venv interpreter as a path relative to the project root (`.venv/bin/python` / `.venv/Scripts/python.exe`) instead of an absolute/joined path, so the generated `{SCRIPT}` invocation stays portable and runnable from the repo root regardless of where the project lives. - Update `install_scripts()` docstring to note `.py` scripts are now made executable alongside `.sh`. - Update tests to assert the repo-relative interpreter path. Assisted-by: GitHub Copilot (model: Claude Opus 4.8, autonomous) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Collaborator
Author
|
Addressed both review comments in 24c0e2f:
Full suite: 3860 passed, 4 skipped; Posted on behalf of @mnriem by GitHub Copilot (model: Claude Opus 4.8, autonomous). |
When neither python3 nor python is discoverable on PATH (and no project
venv is found), resolve_python_interpreter() now returns the running
interpreter (sys.executable) so the generated {SCRIPT} invocation works
in the current environment, falling back to "python3" only if that is
also unavailable. Update unit tests accordingly.
Comment on lines
+627
to
+629
| if script_type == "py": | ||
| interpreter = IntegrationBase.resolve_python_interpreter(project_root) | ||
| script_command = f"{interpreter} {script_command}" |
Comment on lines
+561
to
+564
| Falls back to the running interpreter (``sys.executable``) when | ||
| ``PATH`` resolution fails so the generated command is guaranteed | ||
| to work in the current environment, and finally to ``"python3"`` | ||
| if even that is unavailable. |
For the `py` script type, the resolved interpreter may be an absolute
path containing spaces (notably `sys.executable` under Windows
`Program Files`). Quote it when it contains whitespace so the `{SCRIPT}`
invocation isn't split into multiple arguments. Add positive/negative
tests for the quoting behavior.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Implements #3278 — the foundation step toward unifying workflow scripts under a single Python implementation per command (#3277). Adds a third script variant (
py) alongsidesh/ps.Changes
"py": "Python"toSCRIPT_TYPE_CHOICESin_agent_config.py. AllVALID_SCRIPT_TYPESconsumers (the init workflow step,commands/init.py, andintegrations/_helpers.py) derive from this mapping, so they acceptpywithout further change.process_template()(integrations/base.py): Confirmed the existing genericscript_typelogic extractspy:from frontmatter unchanged. When the script type ispy, the{SCRIPT}expansion is now prefixed with a resolved Python interpreter so.pyscripts run portably (notably on Windows, where.pyfiles aren't directly executable).sh/psgo down the identical path as before.IntegrationBase.resolve_python_interpreter()helper — prefers a project venv (returned relative to the project root:.venv/bin/pythonor.venv/Scripts/python.exe), thenpython3on PATH, thenpythonon PATH, then the running interpreter (sys.executable), falling back topython3only if even that is unavailable.project_rootis threaded through theprocess_template()callers so the venv preference works. When the resolved interpreter path contains whitespace (e.g. WindowsProgram Files) it is quoted so the{SCRIPT}invocation isn't split into multiple args.install_scripts(): Copied.pyfiles are now marked executable (previously.sh-only;.ps1behavior unchanged).Out of scope
Adding
py:lines to the templates'scripts:frontmatter is tracked separately in #3283. Until then,{SCRIPT}is left literal forpy(existing graceful fallback when no matching frontmatter line exists).Tests
Added positive and negative unit tests:
python3, falls back topython, falls back tosys.executablewhen PATH resolution fails (and topython3only if that is empty), prefers POSIX/Windows venv (relative path), ignores a missing venv.process_templatewithpy: prefixes interpreter + rewrites path;shis not prefixed; venv is used whenproject_rootis passed.install_scripts:.py/.shmade executable, non-script file is not.SCRIPT_TYPE_CHOICES/VALID_SCRIPT_TYPES; unknown variant rejected.Verification
specify init x --script pycompletes without error and records"script": "py".ruff checkclean on changed files.🤖 This PR was authored autonomously by GitHub Copilot (model: Claude Opus 4.8) on behalf of @mnriem.