Move create_venv.py to script folder and make it more robust.#865
Move create_venv.py to script folder and make it more robust.#865
Conversation
WalkthroughMove helper scripts into Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
script/create_venv.py (1)
1-68:project_rootis derived robustly, butpip -e .still depends on CWD—confirmed issueThe review comment is accurate. When the script is double-clicked from Windows Explorer (as documented in
doc/contributing.rstline 13), the working directory will bescript/, not the project root. The editable install on line 38 uses-e ., which resolves relative to the current working directory. This causes pip to look forpyproject.tomlinscript/instead of the project root, resulting in installation failure.The batch files (
create_docs.bat,check_quality.bat,tests/run_tests.bat) all change to the project root before invoking the script, so they work. However, the documented double-click usage path does not.The proposed fix is necessary and correct: moving
PROJECT_ROOTto module scope and using an absolute path in the editable install aligns both the venv location and package installation with the script's actual location, making the behavior independent of CWD.
🧹 Nitpick comments (1)
script/create_venv.py (1)
1-5: Consider a more generic shebang for portabilityPinning the shebang to
#!/usr/bin/env python3.12enforces a specific minor version and will fail on systems that only exposepython3(or a different 3.x) onPATH. Since the Windows entry points already usepy, you might consider a more generic shebang likepython3(or even leaving it unused if the script is always invoked viapy/python).Not critical, but it can make the script more portable on Unix‑like systems.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
check_quality.bat(1 hunks)create_docs.bat(1 hunks)doc/contributing.rst(1 hunks)script/create_venv.py(2 hunks)tests/run_tests.bat(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (17)
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: build (ubuntu-latest)
- GitHub Check: DevContainer Build & Test
- GitHub Check: build (windows-latest)
- GitHub Check: test (windows-latest, 3.12)
- GitHub Check: test (macos-latest, 3.14)
- GitHub Check: test (windows-latest, 3.9)
- GitHub Check: test (windows-latest, 3.14)
- GitHub Check: test (macos-latest, 3.9)
- GitHub Check: test (windows-latest, 3.10)
- GitHub Check: test (windows-latest, 3.13)
- GitHub Check: test (macos-latest, 3.13)
- GitHub Check: test (windows-latest, 3.11)
- GitHub Check: test (macos-latest, 3.11)
- GitHub Check: test (macos-latest, 3.10)
- GitHub Check: test
- GitHub Check: test-cygwin
🔇 Additional comments (4)
doc/contributing.rst (1)
13-20: Docs now match script path and extras – just keep pyproject extras in syncThe reference to
script/create_venv.pyand thedevelopment,test,docsextras matches the current script defaults. Please just double‑check that the extras section inpyproject.tomlis actually nameddocsso this text and the tooling stay consistent over time.tests/run_tests.bat (1)
6-6: Launcher and script path update look correct; confirmpyavailabilityUsing
py script/create_venv.pyfromcd %~dp0..correctly targets the repo root and the new script location, and activation still wirespythonto the venv for pytest. Please confirm that relying on the Python launcher (py) is acceptable for all Windows contributors (no environments where onlypythonis available).check_quality.bat (1)
6-6: Consistent venv setup for quality checks; samepyassumptionCalling
py script/create_venv.py --extra_requirements "development"from the repo root is consistent with the new script layout and venv location. As withtests/run_tests.bat, please confirm that all target Windows environments have thepylauncher available.create_docs.bat (1)
6-6: Docs build script aligned with new extras and layout; verifypy+docsextraThe switch to
py script/create_venv.py --extra_requirements "docs"matches the new script location and the renameddocsextra. Please verify that:
- The
docsextra exists inpyproject.toml, and- Using the
pylauncher is acceptable on all Windows setups you care about.
spoorcc
force-pushed
the
cleanup-tooling
branch
from
140ea62 to
8e3b955
Compare
spoorcc
force-pushed
the
cleanup-tooling
branch
from
8e3b955 to
06abcce
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
.devcontainer/Dockerfile(1 hunks).readthedocs.yml(1 hunks)doc/contributing.rst(3 hunks)script/check_quality.bat(1 hunks)script/create_docs.bat(1 hunks)script/create_venv.py(3 hunks)tests/run_tests.bat(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- .devcontainer/Dockerfile
- tests/run_tests.bat
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (16)
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: test
- GitHub Check: test (macos-latest, 3.12)
- GitHub Check: test (ubuntu-latest, 3.13)
- GitHub Check: test (macos-latest, 3.13)
- GitHub Check: test (windows-latest, 3.9)
- GitHub Check: test (macos-latest, 3.10)
- GitHub Check: test (windows-latest, 3.10)
- GitHub Check: test (windows-latest, 3.13)
- GitHub Check: test (windows-latest, 3.14)
- GitHub Check: test (ubuntu-latest, 3.10)
- GitHub Check: test-cygwin
- GitHub Check: DevContainer Build & Test
- GitHub Check: build (macos-latest)
- GitHub Check: build (windows-latest)
- GitHub Check: build (ubuntu-latest)
🔇 Additional comments (7)
script/create_docs.bat (1)
1-10: LGTM!The path adjustments correctly account for the script's new location in
script/. Usingpylauncher is more robust on Windows as it handles version selection automatically.script/check_quality.bat (1)
1-14: LGTM!The changes are consistent with
create_docs.batand correctly adjust paths for the newscript/location.doc/contributing.rst (3)
13-22: LGTM!Documentation correctly updated to reflect the new script paths and Python 3.13 version. The dependency group change from
doctodocsaligns with the default increate_venv.py.
52-52: LGTM!Path correctly updated to
script/check_quality.bat.
91-91: LGTM!Path correctly updated to
script/create_docs.bat.script/create_venv.py (2)
11-22: Good addition of version guards and PROJECT_ROOT.The
PROJECT_ROOTcalculation correctly navigates fromscript/to the project root. The version guards provide clear feedback for unsupported Python versions.
76-87: LGTM!The venv is now correctly created at
PROJECT_ROOT / "venv", ensuring consistent behavior regardless of the current working directory when running the script.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
script/create_venv.py (2)
14-27: Version gating is clear; consider avoiding import‑time side effectsThe min/recommended version checks are straightforward and the messages read well. If you ever expect
MyEnvBuilderto be imported from other tooling (rather than run purely as a script), consider moving the version checks under theif __name__ == "__main__":guard to avoid raisingRuntimeErrorat import time in older interpreters.
54-59: Editable install from PROJECT_ROOT removes CWD dependencySwitching the editable install target to
f"{PROJECT_ROOT}{self.extra_requirements}"correctly installs the top‑level project regardless of the current working directory, and the extras syntax is preserved. If you want to make the intent slightly clearer, you could explicitly cast the path, e.g.f"{PROJECT_ROOT!s}{self.extra_requirements}", but the current form is functionally fine.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
script/create_venv.py(3 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (20)
- GitHub Check: Publish Python distribution 📦 to TestPyPI
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: test (windows-latest, 3.12)
- GitHub Check: test (macos-latest, 3.11)
- GitHub Check: test (windows-latest, 3.14)
- GitHub Check: test (macos-latest, 3.13)
- GitHub Check: test (windows-latest, 3.11)
- GitHub Check: test (windows-latest, 3.9)
- GitHub Check: test (ubuntu-latest, 3.11)
- GitHub Check: test (windows-latest, 3.10)
- GitHub Check: test (macos-latest, 3.9)
- GitHub Check: test (macos-latest, 3.10)
- GitHub Check: test (ubuntu-latest, 3.14)
- GitHub Check: test (windows-latest, 3.13)
- GitHub Check: test
- GitHub Check: test-cygwin
- GitHub Check: build (macos-latest)
- GitHub Check: build (ubuntu-latest)
- GitHub Check: build (windows-latest)
- GitHub Check: DevContainer Build & Test
🔇 Additional comments (2)
script/create_venv.py (2)
1-11: Shebang and PROJECT_ROOT handling improve portabilityUsing
#!/usr/bin/env python3and derivingPROJECT_ROOTfrom__file__makes the script robust to being invoked from arbitrary working directories and across platforms. No issues spotted here.
76-87: CLI defaults and venv location align with project layoutDefaulting
--extra_requirementsto"development,test,docs"and creating the venv atPROJECT_ROOT / "venv"matches the repo‑root‑based tooling and should make editor/CI integration more predictable. Looks good.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
script/create_venv.py (3)
1-15: Shebang, PROJECT_ROOT, and version constants are well-chosenUsing
/usr/bin/env python3plus a runtimeMIN_VERSIONcheck is portable, and computingPROJECT_ROOTfrom__file__avoids cwd-related surprises. Consider, long term, drivingRECOMMENDED_VERSIONfrom a single source (e.g.,pyproject.tomlor config) so it cannot drift from other tooling/docs.
37-46: Editable install via PROJECT_ROOT improves robustness; docstring could be updatedUsing
PROJECT_ROOTfor the editable install target is a good robustness improvement over cwd-relative paths, and the extras handling is consistent with the CLI argument. The only mismatch is the class docstring still referencing arequirements.txt-driven flow; you may want to update it to mention the editable install with optional extras instead.
70-79: Version gating and warning behavior are correct; consider tiny readability tweaksThe minimum-version guard and recommended-version warning behave as intended and make the script’s expectations explicit. If you want to tighten things up, you could cache
current = sys.version_info[:2]to avoid repetition, or switch tosys.exit()for a slightly cleaner error message when the version is too old, but that’s purely cosmetic.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
script/create_venv.py(3 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (16)
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: test (windows-latest, 3.13)
- GitHub Check: test (windows-latest, 3.10)
- GitHub Check: test (windows-latest, 3.11)
- GitHub Check: test (windows-latest, 3.12)
- GitHub Check: test (windows-latest, 3.14)
- GitHub Check: test (macos-latest, 3.14)
- GitHub Check: test (windows-latest, 3.9)
- GitHub Check: test (ubuntu-latest, 3.12)
- GitHub Check: test (macos-latest, 3.9)
- GitHub Check: test-cygwin
- GitHub Check: DevContainer Build & Test
- GitHub Check: build (macos-latest)
- GitHub Check: build (windows-latest)
- GitHub Check: build (ubuntu-latest)
- GitHub Check: test
🔇 Additional comments (2)
script/create_venv.py (2)
81-85: Project-root-based venv path aligns with the tooling/docs changesTargeting
PROJECT_ROOT / "venv"ensures the environment is created consistently at the repo root regardless of where the script is invoked from, which lines up with the PR’s restructuring of scripts and docs.
64-67: All default extras are properly defined in pyproject.tomlThe extras
development,test, anddocsare all present in[project.optional-dependencies]and will install without issues when the venv script runs.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
script/create_venv.py (1)
61-79: CLI default extras and version checks are sensible; consider cleaner error UX.The default
--extra_requirements=development,test,docsand the min/recommended version checks are well-structured and do what they intend. If you want a slightly cleaner CLI experience, you could consider usingsys.exit(<message>)instead of raisingRuntimeErrorfor the unsupported-version case to avoid showing a full stack trace for what is essentially a user-facing compatibility error.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
script/create_venv.py(3 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (19)
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: test (windows-latest, 3.14)
- GitHub Check: test (windows-latest, 3.10)
- GitHub Check: test (windows-latest, 3.13)
- GitHub Check: test (windows-latest, 3.9)
- GitHub Check: test (windows-latest, 3.12)
- GitHub Check: test (macos-latest, 3.9)
- GitHub Check: test (windows-latest, 3.11)
- GitHub Check: test (ubuntu-latest, 3.9)
- GitHub Check: test (ubuntu-latest, 3.13)
- GitHub Check: test (macos-latest, 3.10)
- GitHub Check: test (ubuntu-latest, 3.14)
- GitHub Check: test (ubuntu-latest, 3.10)
- GitHub Check: test-cygwin
- GitHub Check: DevContainer Build & Test
- GitHub Check: build (ubuntu-latest)
- GitHub Check: build (macos-latest)
- GitHub Check: build (windows-latest)
- GitHub Check: test
🔇 Additional comments (3)
script/create_venv.py (3)
1-15: PROJECT_ROOT and version constants look good; ensure they stay aligned with supported matrix.The
PROJECT_ROOTresolution and theMIN_VERSION/RECOMMENDED_VERSIONtuple approach are solid and clear. Just make sure these version constants stay in sync withpyproject.toml’srequires-pythonand your CI matrix so users don’t see contradictory support vs. tooling messages.
35-44: Editable install via PROJECT_ROOT improves robustness.Switching to
PROJECT_ROOTinpost_setup()so pip installs-e <abs_project_root>[extras]is a nice robustness improvement (no dependency on CWD) and keeps extras handling straightforward. No issues from my side here.
81-85: Venv location tied to PROJECT_ROOT is a good hardening step.Targeting
PROJECT_ROOT / "venv"makes the environment creation independent of the caller’s working directory and should behave more predictably across tooling and docs. This change looks correct.
2 participants
Summary by CodeRabbit
Chores
Documentation
✏️ Tip: You can customize this high-level summary in your review settings.