fix: prefer current workspace for update - #57
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
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 |
abhiksark
left a comment
There was a problem hiding this comment.
Welcome to Pythonlings, @Huoyanlifusu, and thank you for your first contribution! The precedence implementation is focused and behaves correctly in my review: explicit update --path, global --root, current workspace, and default home workspace resolve in the required order, while a missing target fails without being created. The full suite also passes (python -m pytest -q: 147 passed), as does pythonlings --root tests/fixtures/passing_curriculum verify. Before merge, please add the integration tests promised in issue #42 for every precedence branch, including the missing/non-workspace no-creation case. The PR currently changes only pythonlings/cli.py, and its checklist correctly shows test coverage and the required full-suite command as incomplete. Please update those checks and record the exact results in the description after adding the regressions.
|
Thanks for the review! I added the requested integration coverage — 6 new tests in The new tests cover every precedence branch:
Verification:
Rebased onto the latest |
e963463 to
9e42d72
Compare
9e42d72 to
0aa40d7
Compare
abhiksark
left a comment
There was a problem hiding this comment.
The requested precedence regressions are present, invalid targets are rejected before mutation, and the current head passes the full Python 3.11 through 3.13 CI matrix, including package build, wheel reinstall, and installed CLI flows. Approved for squash merge into dev.
Fixes #42
Summary
update --pathwas explicitly provided--path, global--root, current workspace, then the default home workspacePreviously, bare
pythonlings updatealways received the default home path from argparse, so it could update~/.pythonlingseven when run from another valid workspace.Tests
Integration coverage verifies every precedence branch and the failure boundaries:
update --pathwins over global--rootand the current workspace--rootwins over the current workspaceupdateselects the current workspaceupdatefalls back to the default home workspace outside a workspaceValidation after rebasing onto current
dev:python -m pytest tests/integration/test_cli_workspace.py tests/unit/test_workspace.py -q23 passedpython -m pytest -q185 passedpythonlings --root tests/fixtures/passing_curriculum verifypython -m ruff check pythonlings/cli.py pythonlings/core/curriculum.py tests/integration/test_cli_workspace.pyPIP_CONFIG_FILE=/dev/null PIP_INDEX_URL=https://pypi.org/simple python -m buildpythonlings-0.4.1.tar.gzandpythonlings-0.4.1-py3-none-any.whlpythonlings updatepythonlings --versionreported0.4.1Screenshots
Not applicable. This changes CLI workspace resolution only.
Checklist