Stability Fixes - #15
Conversation
erezh16
commented
Jun 14, 2026
- Removed and ignored .stamps
- Fixed ci-pull-request target name
- Added venv creation to CI workflow
- Added venv creation to push workflow as well
- Reactivate venv at run_make stage
- Fixed a failing test
- Lint fix
|
This PR is only "infrastructure stuff". All tests passing. |
eranra
left a comment
There was a problem hiding this comment.
PR #15 — Stability Fixes Review
The vast majority (~95%) is pure Black formatter output — double quotes, trailing commas, line-length wrapping. Here's what actually matters:
Real Changes Worth Reviewing
- CI Workflow fixes (.github/workflows/pull_request.yml, .github/workflows/push.yml) — Important
Added python -m venv .venv + pip install steps before make
Fixed make target names: ci_pull_request → ci-pull-request, ci_push → ci-push
source .venv/bin/activate is called in each step independently — this is correct for GitHub Actions (each run: block is a new shell, so you must re-activate per step). No issue here.
2. Test bug fix (config/test_config_integration.py:354) — Important
test_load_no_env_vars now clears all SPA_* env vars before running. Previously it could fail spuriously if the test runner had SPA_* vars set in the environment. Good fix.
3. Indentation fix (utils/utils.py:55) — Small but real
recurse(main_key, value) was indented one extra level inside the for loop. This was a logic bug that would have caused recurse to only be called once (on the last iteration body context). Real fix.
4. Gitignore + deleted .stamps/ files — Housekeeping, no logic risk.
One Remaining Smell (Not Fixed)
In utils/utils.py:105, the bare raise with no active exception:
if not sub_parts:
raise # ← bare raise outside except block
This will throw RuntimeError: No active exception to re-raise at runtime when sub_parts is empty, then caught by the except: below. It works, but it's fragile. It was this way before this PR too — not introduced here, just reformatted.