You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
{{ message }}
Repository navigation
tests/test_adopt_plan_value_order.py: follow-up of PR #1226 (#1179) from its review: scripts/ goes on sys.path a second time for an import test_adopt_template_values already makes, and the module docstring names two of _work_root's refusals as "two more" #1234
Found by: RV1226, lane 4's independent review of PR #1226 (for #1179) at f61be9f5 (#1226 issuecomment-6374526997, #1226 (comment), posted 2026-10-23T20:53:40Z). It graded #1226READY, with no MUST and no SHOULD, and left two NITs for follow-ups: NIT-1 and NIT-2, which are this issue's. #1226 is READY at f61be9f5 and lands without these. This follows #1179 and #1226.
Line numbers are at f61be9f5 (#1226's head, one commit on #1125's head 9549006fe), read from that commit for this issue and not run. #1226 is a draft whose base is fix/1020-bad-id-first-problem (#1125's branch), and #1125 is a draft against main, so neither is on main and this goes onto a branch after #1226 lands. The file is tests/test_adopt_plan_value_order.py only. adopt-project.py is sha256 412edbda on #1125's head and on #1226's, and neither NIT touches it.
What is missing
Nothing a user can see, and no row that passes for a wrong reason: both NITs are about the test file's own text. RV1226 read _work_root, _made_0755, cmd_check and cmd_execute, and each of #1226's six rows holds what #1179 asks: each control proves its premise, and each of RV1226's 13 mutants fails the new rows it should. Four of them (L1, L2, L4 and L11) survive all 85 old rows, and so does L8, which makes both commands ask late. Nothing was re-run for this issue.
NIT-1: a second sys.path insert for an import that is already loaded
But test_adopt_template_values, imported a few lines above (about :81, from test_adopt_template_values import with_line), already does both at its import: its sys.path.insert(0, str(REPO / "scripts")) and from repo_shape import double_quoted, load_yaml (lines 57 and 59 at #1226's head, per RV1226; 55 and 57 on main). So scripts/ lands on sys.path twice. sys and REPO are used nowhere else in the file (read for this issue: a grep of the file at f61be9f5 finds sys only in its import and in that insert, and REPO only in the conftest import and in that insert).
Change, RV1226's:from test_adopt_template_values import double_quoted, with_line. Then drop the sys import, the REPO import and the insert.
NIT-2: the module docstring undercounts _work_root's refusals
The docstring (about :55) says: "_work_root refuses two more things before it makes anything, a --work-dir that is a file and one every user can write in". _work_root also refuses:
a symlink loop;
a symlink to something that is no directory;
a directory another user owns;
a shared directory above the --work-dir.
The rows rightly cover two of them (the file and the shared directory); #1179 asks for those two.
Change, RV1226's: "refuses more before it makes anything, among them a --work-dir that is a file and one every user can write in". Or leave it.
Done when
NIT-1: the file puts nothing on sys.path itself and imports double_quoted and with_line from test_adopt_template_values, with sys and REPO no longer imported. The 91 rows of the two order files that RV1226 collected on the head (the six new ids among them) still collect and pass.
NIT-2: the docstring no longer counts _work_root's refusals as two (it says "among them", as RV1226 words it), or a line here says it is left as it is.
Activity