From 04b933149bac8bd51c6096550f58e1ae001032e1 Mon Sep 17 00:00:00 2001 From: Brett Heap Date: Fri, 16 Oct 2026 13:13:55 +0000 Subject: [PATCH 1/2] adopt-project.py: `plan` refuses `--id ''` and `--elected-by ''` by name, a derived id's refusal says to change `--project`, or in place to rename the source first, and an org read from a url is named as not given (#658) `plan` read the flags it fills in by itself by their truth (`flags_given`) and filled them in with `or`, so a passed `''` was the same as no flag (L2-53). `--id ''` planned the derived id, exit 0: a script's `--id "$ID"` with `ID` unset went through silently. `--elected-by ''` recorded the configured `git config user.name`, exit 0, though `--help` says "not blank", and with no `user.name` the refusal said `--elected-by (not given, so `git config user.name`) is ''` of a flag the person had passed. With #620, `--project North_Wind --id ''` was refused as "(--id was not given)". - `flags_given` is now `getattr(args, key) is not None`, as #479 reads `--tracking-branch`, and the id and the elector are filled in only when their flag is not in it. An empty value meets its own flag's rule and is refused there by name, with the value as typed: `adopt-bad-id: --id '' must match ^[a-z0-9][a-z0-9-]*$`, and `adopt-manifest-invalid: --elected-by is '', which the project's own validate-manifest.py refuses: manifest-elected-by: elected_by is empty.`, whatever `user.name` holds. #479 has no refusal of its own for `''` either: `--tracking-branch ''` meets `checked_value` because it is read by `is not None`. A derived id's remedy was "pass an explicit lowercase --id" (L2-54). The id is derived from `--project`, it is refused only for a `--project` holding a character no id takes, and the naming policy reads `--project` itself, so that remedy led to `naming-unclassified`. - `_refuse_bad_id` names the derived id by `_flag_named` from `FLAG_DEFAULTS["id"]`, replacing #620's hand-built sentence: `--id (not given, so --project lowercased) is 'north_wind', which must match ...`. Its remedy is "change --project. The id is --project lowercased, and an explicit --id would not change --project, which also names the assembly root and both legs." Where the source names a repository and `--project` is its name, compared without case, it is adopted in place and cannot be retyped: the remedy is `_rename_first`'s, after "the id is --project lowercased, and --project is the source's own name." A passed `--id` keeps its sentence and remedy. The rider, lane 1's L1-39: `adopt-manifest-invalid` named an organisation read from the source's `origin` as `--org is '~x'`, a flag nobody passed. `FLAG_DEFAULTS` gains `"org": "the organisation of the source's `origin`"`, `_planned_org`'s words for the same value, so the validator's refusal says `--org (not given, so the organisation of the source's `origin`) is '~x'`. Not changed: `--org ''`, which `_planned_org` still reads as no `--org`; `--elected-on ''` and `--reference ''`, which are not in `FLAG_DEFAULTS`; the scaffold's and `family.py init`'s id refusals (#659); `check`'s `plan-bad-id`. `--help` is unchanged. Lands after #620. Lane: openreposhape-2 (openRepoShape-2) Co-Authored-By: Claude Opus 5.5 --- adopt-project.py | 78 +++++++++++++++------ tests/test_adopt_git_reads.py | 26 +++++++ tests/test_adopt_manifest_values.py | 27 ++++++-- tests/test_adopt_origin_repository.py | 22 ++++++ tests/test_adopt_plan.py | 97 +++++++++++++++++++++------ 5 files changed, 201 insertions(+), 49 deletions(-) diff --git a/adopt-project.py b/adopt-project.py index a60fa24a..d8c49e5e 100755 --- a/adopt-project.py +++ b/adopt-project.py @@ -2665,10 +2665,16 @@ def _kept_collision_notes(args, entries: list, tree: list) -> list[str]: #: and no elector is one: the manifest writes it between double quotes, which #: `render` spells with `double_quoted` (#284). The validator's rule (#261) #: asks about every value, so an elector `git config user.name` leaves empty -#: is named by where it came from as well (#529). +#: is named by where it came from as well (#529), and so is an organisation +#: read from the source's `origin` (L1-39): a url can end in `~x`, which +#: `checked_value` takes and the validator refuses, and that refusal said +#: `--org is '~x'`. The words are `_planned_org`'s for the same value; a +#: source `plan` cloned has the `--source` it was cloned from as its +#: `origin`. FLAG_DEFAULTS = {"id": "--project lowercased", TRACKING_KEY: "the source's default branch", - ELECTED_BY_KEY: "`git config user.name`"} + ELECTED_BY_KEY: "`git config user.name`", + "org": "the organisation of the source's `origin`"} #: How a sentence names the id `execute` derives when the plan sets none: #: when it has no `id:`, or an `id:` with no value (`null`, `~` or nothing, #: as `main`'s `plan --id null` wrote it), which `Plan.get` reads alike. @@ -3303,22 +3309,44 @@ def _refuse_a_tracking_branch_the_remote_lacks(urls: dict, tracking: str, REMOTE_LACKS_REMEDIATION + (LOCAL_REMOTE_LACKS if local else "")) -def _refuse_bad_id(project_id: str, project: str, passed: bool) -> None: +def _refuse_bad_id(project_id: str, project: str, given: set, + source: Source) -> None: """`plan`'s refusal of an id the project's validators would not take - (`PROJECT_ID_RE`), in the words of where it came from. A `--id` the person - passed is named as the flag they typed, with the value as typed. One - `plan` derived from `--project` (lowercased, `FLAG_DEFAULTS`) is named as - derived, the way `_flag_named` names the tracking branch and the elector - it fills in (#529): `--id 'north_wind' must match` named a flag nobody - had passed (#601). The finding's code is the same for both.""" + (`PROJECT_ID_RE`), in the words of where it came from. `given` is + `flags_given`. A `--id` the person passed, `''` among them (L2-53), is + named as the flag they typed, with the value as typed, and the remedy is + another `--id`. One `plan` derived from `--project` is named by + `_flag_named` from `FLAG_DEFAULTS`, as the tracking branch and the + elector it fills in are (#529): `--id 'north_wind' must match` named a + flag nobody had passed (#601). The finding's code is the same for both. + + A DERIVED ID IS MENDED IN `--project`, NOT WITH `--id` (L2-54). It is + refused only for a `--project` holding a character no id takes, and the + naming policy reads `--project` itself, as all three repositories' + names: "pass an explicit lowercase --id" led to `naming-unclassified`. + So the remedy is to change `--project`; and where the source names a + repository and `--project` is its name, adopted in place, that name is + not the person's to retype, and the remedy is `_rename_first`'s.""" if PROJECT_ID_RE.match(project_id): return - pattern = PROJECT_ID_RE.pattern - what = (f"--id {project_id!r} must match {pattern}" if passed - else f"the id derived from --project {project!r} (--id was not " - f"given) is {project_id!r}, which must match {pattern}") - raise Refusal("adopt-bad-id", what, - "Remediation: pass an explicit lowercase --id.") + flag, pattern = _flag_named("id", given), PROJECT_ID_RE.pattern + if "id" in given: + raise Refusal("adopt-bad-id", + f"{flag} {project_id!r} must match {pattern}", + "Remediation: pass an explicit lowercase --id.") + derived = FLAG_DEFAULTS["id"] + repository = source.repository + if repository is not None and _is_the_source( + repository, repository.partition("/")[0], project): + remedy = _rename_first(repository, f"the id is {derived}, and " + "--project is the source's own name. ") + else: + remedy = (f"Remediation: change --project. The id is {derived}, and " + "an explicit --id would not change --project, which also " + "names the assembly root and both legs.") + raise Refusal("adopt-bad-id", + f"{flag} is {project_id!r}, which must match {pattern}", + remedy) def _checked_plan_inputs(args, source: Source, @@ -3335,7 +3363,15 @@ def _checked_plan_inputs(args, source: Source, # Which of the flags `plan` fills in by itself were passed: a value it # filled in is named by where it came from, should it be refused below. # Not `given`, which #479 binds below to whether `--tracking-branch` was. - flags_given = {key for key in FLAG_DEFAULTS if getattr(args, key)} + # PASSED IS ON THE COMMAND LINE, `''` INCLUDED (L2-53), as #479 reads + # `--tracking-branch`: `--id ''` and `--elected-by ''` were read as not + # passed, so `--id "$ID"` with `ID` unset planned the derived id, and an + # empty elector became `git config user.name` though `--help` says "not + # blank". Each is a value typed, held to its flag's own rule below and + # refused there by name. (`_planned_org` still reads `--org ''` as no + # `--org`, as it did.) + flags_given = {key for key in FLAG_DEFAULTS + if getattr(args, key) is not None} args.project = checked_value("--project", args.project) # THE SOURCE'S OWN BRANCH, UNLESS ONE IS NAMED (#479): the branch `plan` # reads the source at is the one its pull request can be opened against. @@ -3378,10 +3414,9 @@ def _checked_plan_inputs(args, source: Source, args.spec_path = checked_value(*spec) args.code_path = checked_value(*code) # Whether `--id` was passed is read BEFORE it is filled in: a refusal of - # the id names where it came from (#601). - id_passed = "id" in flags_given - args.id = args.id or args.project.lower() - _refuse_bad_id(args.id, args.project, id_passed) + # the id names where it came from (#601), and says what to change. + args.id = args.id if "id" in flags_given else args.project.lower() + _refuse_bad_id(args.id, args.project, flags_given, source) # The organisation, given or read from the source's `origin`, and named # by which of the two it is should it be refused (#292, #427). org_given = bool(args.org) @@ -3400,7 +3435,8 @@ def _checked_plan_inputs(args, source: Source, # A value `plan` filled in by itself is named by where it came from, as # #181 names one, and not as a flag the person never passed. elector = _flag_named(ELECTED_BY_KEY, flags_given) - args.elected_by = args.elected_by or _elector() + args.elected_by = (args.elected_by if ELECTED_BY_KEY in flags_given + else _elector()) # Before the plan is written, in the words `check` would use (#255): a # value `execute` writes into a file must be text one can hold. The # date has had its answer above, `election_date`. diff --git a/tests/test_adopt_git_reads.py b/tests/test_adopt_git_reads.py index a327e56f..90157c35 100644 --- a/tests/test_adopt_git_reads.py +++ b/tests/test_adopt_git_reads.py @@ -292,6 +292,32 @@ def test_plan_records_the_elector_given_where_user_name_is_unset(tmp_path): assert load_yaml(tmp_path / PLAN_FILE)["elected_by"] == "Test Human" +#: What `plan` says of a `--elected-by ''` the person passed (L2-53): the +#: flag as typed, in the validator's words for an empty elector. +EMPTY_ELECTOR = (b"REFUSED adopt-manifest-invalid: --elected-by is '', which " + b"the project's own validate-manifest.py refuses: " + b"manifest-elected-by: elected_by is empty.") + + +@pytest.mark.parametrize("user_name", ["unset", "configured"]) +def test_plan_refuses_an_empty_elector_that_was_passed(tmp_path, user_name): + """L2-53. `--elected-by ''` was read as not passed: with a `git config + user.name` the plan recorded that name, exit 0, though `--help` says + "not blank"; with none, the refusal said `--elected-by (not given, so + `git config user.name`) is ''` of a flag the person passed. It is the + flag's own value now, refused by name as a blank one is (#261), and + `git config user.name` does not stand in for it.""" + extra = (no_user_name(tmp_path, "unset") if user_name == "unset" + else global_user_name(tmp_path, b"Jose")) + source = make_source_repo(tmp_path / "Thing") + refused = by_git_config(tmp_path, source, extra, "--elected-by", "") + assert_no_traceback_and_no_leg(tmp_path, {"plan": refused}) + assert refused.returncode == 2, said(refused) + assert EMPTY_ELECTOR in refused.stderr, said(refused) + assert b"not given" not in refused.stderr, said(refused) + assert not (tmp_path / PLAN_FILE).exists() + + @pytest.mark.parametrize("name", [b"Jose", "José".encode("utf-8")], ids=["ascii", "utf8"]) def test_plan_records_a_user_name_that_is_utf8(tmp_path, name): diff --git a/tests/test_adopt_manifest_values.py b/tests/test_adopt_manifest_values.py index a5e54b4e..6d4b70bc 100644 --- a/tests/test_adopt_manifest_values.py +++ b/tests/test_adopt_manifest_values.py @@ -407,14 +407,16 @@ def test_the_flags_and_the_plan_keys_name_the_same_values(adopter): def test_a_value_plan_filled_in_is_named_by_where_it_came_from(adopter): - """#529: the three values `plan` fills in when its flag is not given are - named by the default, in the words `_flag_fields` and the unwritable-value - path use, and no other value is renamed. Two of the three (the id, the - tracking branch) are refused before the validator is asked, so this is - where their names are held.""" + """#529 and L1-39: the four values `plan` fills in when its flag is not + given are named by the default, in the words `_flag_fields` and the + unwritable-value path use, and no other value is renamed. Two of the four + (the id, the tracking branch) are refused before the validator is asked, + so this is where their names are held. The organisation is in + `_planned_org`'s words: `--org is '~x'` named a flag nobody passed.""" defaults = {"ELECTED_BY": "`git config user.name`", "PROJECT_ID": "--project lowercased", - "TRACKING_BRANCH": "the source's default branch"} + "TRACKING_BRANCH": "the source's default branch", + "ORG": "the organisation of the source's `origin`"} typed = adopter._manifest_flag_names(set(adopter.FLAG_DEFAULTS)) filled = adopter._manifest_flag_names(set()) assert filled == { @@ -427,6 +429,19 @@ def test_a_value_plan_filled_in_is_named_by_where_it_came_from(adopter): "--elected-by") +def test_the_derived_ids_words_are_the_tables(adopter, monkeypatch): + """rv601's nit, L2-54: #620 wrote the derived id's sentence by hand, so + an edit to `FLAG_DEFAULTS["id"]` would not have reached it. Its label + and its remedy are both read from the table.""" + monkeypatch.setitem(adopter.FLAG_DEFAULTS, "id", "a stand-in") + with pytest.raises(adopter.Refusal) as refused: + adopter._refuse_bad_id("north_wind", "North_Wind", set(), + Namespace(repository=None)) + assert refused.value.detail.startswith( + "--id (not given, so a stand-in) is 'north_wind'"), refused.value + assert "The id is a stand-in, and" in refused.value.remediation + + # --- the question is asked with no git at all ------------------------------- # # The manifest is rendered here to be ASKED ABOUT and never written: the diff --git a/tests/test_adopt_origin_repository.py b/tests/test_adopt_origin_repository.py index 03026889..dc3ba593 100644 --- a/tests/test_adopt_origin_repository.py +++ b/tests/test_adopt_origin_repository.py @@ -251,6 +251,28 @@ def test_an_org_read_from_origin_is_refused_by_where_it_came_from( assert not (tmp_path / PLAN).exists() +@pytest.mark.parametrize("org, named", [(None, ORG_FROM_ORIGIN), + ("~x", "--org")], + ids=["read-from-origin", "passed"]) +def test_an_org_the_validator_refuses_is_named_by_where_it_came_from( + tmp_path, org, named): + """L1-39, #427's pattern in the validator's refusal: a url can end in an + organisation `checked_value` takes and the project's own validator + refuses, `~x`, and with no `--org` that refusal said `--org is '~x'`, a + flag nobody passed. It names the value by where it came from, in the + words `checked_value`'s refusal above uses; a `--org ~x` that was + passed is named as the flag.""" + source = with_origin(tmp_path, f"https://example.invalid/~x/{OTHER}.git") + + refused = planned(tmp_path, source, OTHER, org) + + assert refused.returncode == 2, refused.stdout + refused.stderr + assert refused.stderr.splitlines()[0].startswith( + f"REFUSED adopt-manifest-invalid: {named} is '~x', which the " + "project's own validate-manifest.py refuses: "), refused.stderr + assert not (tmp_path / PLAN).exists() + + @pytest.mark.parametrize("directory", ["caf\u00e9", "my repos"], ids=["not-ascii", "space"]) def test_a_path_origin_names_no_organisation(tmp_path, directory): diff --git a/tests/test_adopt_plan.py b/tests/test_adopt_plan.py index 3aaae6c1..1e4b46d0 100644 --- a/tests/test_adopt_plan.py +++ b/tests/test_adopt_plan.py @@ -16,7 +16,8 @@ import pytest -from conftest import ADOPT, REPO, resolve, run_script, write_plan +from conftest import ADOPT, REPO, git, resolve, run_script, write_plan +from test_adopt_project_is_the_source import RENAME_FIRST sys.path.insert(0, str(REPO / "scripts")) from path_classify import PathPolicy # noqa: E402 @@ -371,38 +372,91 @@ def test_a_project_name_that_is_not_a_lowercase_id_is_refused(source_repo, #: `repo_shape.PROJECT_ID_RE`, as `adopt-bad-id` prints it. ID_PATTERN = "^[a-z0-9][a-z0-9-]*$" +#: What a passed `--id` is told to do: pass another. +PASS_AN_ID = "Remediation: pass an explicit lowercase --id." +#: How `adopt-bad-id` names an id `plan` derived from `--project`: +#: `_flag_named` over `FLAG_DEFAULTS`, as the tracking branch and the +#: elector are named (#529). +DERIVED_ID = "--id (not given, so --project lowercased)" +#: And what it is told to do (L2-54): change `--project`, which the naming +#: policy reads itself, so "pass an explicit lowercase --id" led to +#: `naming-unclassified`. +CHANGE_PROJECT = ( + "Remediation: change --project. The id is --project lowercased, and an " + "explicit --id would not change --project, which also names the " + "assembly root and both legs.") +#: Or, where `--project` is the name of the repository the source names, +#: adopted in place, `_rename_first`'s words. +IN_PLACE = ("Remediation: the id is --project lowercased, and --project is " + f"the source's own name. testorg/North_Wind {RENAME_FIRST}") + + +def bad_id(result) -> list[str]: + """The first two lines `plan` printed on stderr, the refusal and its + remediation, once it exited 2.""" + assert result.returncode == 2, result.stdout + result.stderr + return result.stderr.splitlines()[:2] def test_a_passed_id_that_is_not_a_lowercase_id_is_named_as_passed( source_repo, tmp_path): """#601: the refusal of a `--id` the person typed keeps naming that flag, - with the value as typed; nothing in it says the id came from elsewhere.""" + with the value as typed, and its remedy is another `--id`; nothing in it + says the id came from elsewhere.""" plan = tmp_path / "plan.yaml" result = write_plan(source_repo, plan, project="Northwind", extra=("--id", "Foo_Bar")) - assert result.returncode == 2, result.stdout + result.stderr - assert (f"REFUSED adopt-bad-id: --id 'Foo_Bar' must match {ID_PATTERN}" - in result.stderr), result.stderr - assert "derived" not in result.stderr, result.stderr + assert bad_id(result) == [ + f"REFUSED adopt-bad-id: --id 'Foo_Bar' must match {ID_PATTERN}", + PASS_AN_ID], result.stderr + assert not plan.exists() + + +@pytest.mark.parametrize("project", ["Northwind", "North_Wind"]) +def test_an_empty_id_is_refused_as_the_id_passed(source_repo, tmp_path, + project): + """L2-53: `--id ''` was read as not passed, so a script's `--id "$ID"` + with `ID` unset planned `id: northwind`, exit 0, and beside `--project + North_Wind` was refused as an id derived from it, "(--id was not given)" + (rv601's P4). It is a value typed, held to the id's rule as any other + and refused by name, as `--tracking-branch ''` is since #479.""" + plan = tmp_path / "plan.yaml" + result = write_plan(source_repo, plan, project=project, + extra=("--id", "")) + assert bad_id(result) == [ + f"REFUSED adopt-bad-id: --id '' must match {ID_PATTERN}", + PASS_AN_ID], result.stderr assert not plan.exists() -@pytest.mark.parametrize("project", ["North_Wind", "Wind.Mill"]) +@pytest.mark.parametrize("origin, project, remediation", [ + (None, "North_Wind", CHANGE_PROJECT), + (None, "Wind.Mill", CHANGE_PROJECT), + ("testorg/Atlas", "North_Wind", CHANGE_PROJECT), + ("testorg/North_Wind", "North_Wind", IN_PLACE), + ("testorg/North_Wind", "north_wind", IN_PLACE), +], ids=["underscore", "dot", "another-repository", "in-place", + "in-place-other-case"]) def test_an_id_derived_from_the_project_is_named_as_derived( - source_repo, tmp_path, project): + source_repo, tmp_path, origin, project, remediation): """#601: with no `--id`, `plan` lowercases `--project` into the id, and - the refusal of that id names where it came from, as `_flag_named` names - the tracking branch and the elector (#529), and not as a flag nobody - passed: the sentence used to read `--id 'north_wind' must match`.""" + the refusal of that id names where it came from, by `_flag_named`, as + the tracking branch and the elector are named (#529), and not as a flag + nobody passed (`--id 'north_wind' must match`). + + L2-54: and it says to change `--project`, not to pass an `--id`. Where + the source names a repository and `--project` is its name, compared + without case, it is adopted in place and cannot be retyped, so the + remedy is to rename it first. A source naming another repository is + told to change `--project` (`adopt-not-the-source` names its own).""" + if origin is not None: + git("remote", "add", "origin", f"https://github.com/{origin}.git", + cwd=source_repo) plan = tmp_path / "plan.yaml" result = write_plan(source_repo, plan, project=project) - derived = project.lower() - assert result.returncode == 2, result.stdout + result.stderr - assert ("REFUSED adopt-bad-id: the id derived from --project " - f"{project!r} (--id was not given) is {derived!r}, which must " - f"match {ID_PATTERN}") in result.stderr, result.stderr - assert f"--id {derived!r}" not in result.stderr, result.stderr - assert "Remediation: pass an explicit lowercase --id." in result.stderr + assert bad_id(result) == [ + f"REFUSED adopt-bad-id: {DERIVED_ID} is {project.lower()!r}, which " + f"must match {ID_PATTERN}", remediation], result.stderr assert not plan.exists() @@ -414,10 +468,9 @@ def test_a_passed_id_equal_to_the_derived_one_is_still_named_as_passed( plan = tmp_path / "plan.yaml" result = write_plan(source_repo, plan, project="North_Wind", extra=("--id", "north_wind")) - assert result.returncode == 2, result.stdout + result.stderr - assert (f"REFUSED adopt-bad-id: --id 'north_wind' must match {ID_PATTERN}" - in result.stderr), result.stderr - assert "derived" not in result.stderr, result.stderr + assert bad_id(result) == [ + f"REFUSED adopt-bad-id: --id 'north_wind' must match {ID_PATTERN}", + PASS_AN_ID], result.stderr assert not plan.exists() From f06d3e65b210622e3d31be23efef9f6cff5cd8ab Mon Sep 17 00:00:00 2001 From: Brett Heap Date: Fri, 16 Oct 2026 14:22:36 +0000 Subject: [PATCH 2/2] adopt-project.py: a derived id's remedy names the rule `naming-unclassified` gives for `--project`, so "change --project" no longer leads to `North-Wind` and a second refusal (#658) rv699's should-fix (lane 4's O1). The remedy said "change --project", and the change a person makes to `North_Wind` is `North-Wind`, which `naming-unclassified` then refused: "--project takes one CamelCase token with no hyphen, underscore, dot or space". - `PROJECT_NAME_RULE` holds that clause once. `_check_names` builds `naming-unclassified`'s remediation from it, byte for byte as before, and `_refuse_bad_id` names it in both derived remedies: "Remediation: change --project to one CamelCase token with no hyphen, underscore, dot or space. The id is --project lowercased, ..." and, in place, "the id is --project lowercased, and --project is the source's own name, which must be one CamelCase token with no hyphen, underscore, dot or space." before `_rename_first`'s text, unchanged. - tests/test_adopt_plan.py: `CHANGE_PROJECT` and `IN_PLACE` say the rule, and a new test holds the derived remedy and the `naming-unclassified` that follows `--id north-wind` to the same clause. - tests/test_adopt_project_is_the_source.py: a cloned `--source testorg/North_Wind` gets the rename-first remedy, as a local clone does (rv699's X1: `repository = None if source.cloned else ...` survived). The `gh repo clone` stand-in is one helper for that test and #499's. - tests/test_naming_policy.py: no assembly root and no family holder holds `.`, `_`, `/`, `@`, `+` or `~`, the characters `SAFE_ARG_RE` takes and no id takes once lowercased; a second test holds that list to the two patterns. The in-place remedy says the source cannot be adopted under such a name, and #659 says the same of `--family`: true while these hold. Lane: openreposhape-2 (openRepoShape-2) Co-Authored-By: Claude Opus 5.5 --- adopt-project.py | 30 ++++++++++----- tests/test_adopt_plan.py | 35 +++++++++++++---- tests/test_adopt_project_is_the_source.py | 46 ++++++++++++++++++++--- tests/test_naming_policy.py | 34 ++++++++++++++++- 4 files changed, 120 insertions(+), 25 deletions(-) diff --git a/adopt-project.py b/adopt-project.py index d8c49e5e..4556671b 100755 --- a/adopt-project.py +++ b/adopt-project.py @@ -3309,6 +3309,14 @@ def _refuse_a_tracking_branch_the_remote_lacks(urls: dict, tracking: str, REMOTE_LACKS_REMEDIATION + (LOCAL_REMOTE_LACKS if local else "")) +#: The name the naming policy takes for an assembly root, in the words +#: `naming-unclassified` says it in (`_check_names`). A derived id's remedy +#: says the same (#658): "change --project" alone sent the person to +#: `North-Wind`, which `naming-unclassified` then refused. +PROJECT_NAME_RULE = ("one CamelCase token with no hyphen, underscore, dot or " + "space") + + def _refuse_bad_id(project_id: str, project: str, given: set, source: Source) -> None: """`plan`'s refusal of an id the project's validators would not take @@ -3324,9 +3332,12 @@ def _refuse_bad_id(project_id: str, project: str, given: set, refused only for a `--project` holding a character no id takes, and the naming policy reads `--project` itself, as all three repositories' names: "pass an explicit lowercase --id" led to `naming-unclassified`. - So the remedy is to change `--project`; and where the source names a - repository and `--project` is its name, adopted in place, that name is - not the person's to retype, and the remedy is `_rename_first`'s.""" + So the remedy is to change `--project`, to what the naming policy takes + (`PROJECT_NAME_RULE`, the words of `naming-unclassified`); and where the + source names a repository and `--project` is its name, adopted in place, + that name is not the person's to retype, and the remedy is + `_rename_first`'s, after the same rule. A source `plan` cloned names its + repository as an `origin` does (`Source.repository`).""" if PROJECT_ID_RE.match(project_id): return flag, pattern = _flag_named("id", given), PROJECT_ID_RE.pattern @@ -3339,11 +3350,13 @@ def _refuse_bad_id(project_id: str, project: str, given: set, if repository is not None and _is_the_source( repository, repository.partition("/")[0], project): remedy = _rename_first(repository, f"the id is {derived}, and " - "--project is the source's own name. ") + "--project is the source's own name, which " + f"must be {PROJECT_NAME_RULE}. ") else: - remedy = (f"Remediation: change --project. The id is {derived}, and " - "an explicit --id would not change --project, which also " - "names the assembly root and both legs.") + remedy = (f"Remediation: change --project to {PROJECT_NAME_RULE}. " + f"The id is {derived}, and an explicit --id would not " + "change --project, which also names the assembly root and " + "both legs.") raise Refusal("adopt-bad-id", f"{flag} is {project_id!r}, which must match {pattern}", remedy) @@ -11258,8 +11271,7 @@ def _check_names(policy: NamingPolicy, names: dict[str, str], "naming-unclassified", f"{name!r} matches no family in the naming policy", renamed or - "Remediation: --project takes one CamelCase token with no " - "hyphen, underscore, dot or space.") + f"Remediation: --project takes {PROJECT_NAME_RULE}.") if not accepts_role(found, role): raise Refusal( "naming-role-mismatch", diff --git a/tests/test_adopt_plan.py b/tests/test_adopt_plan.py index 1e4b46d0..79a1d810 100644 --- a/tests/test_adopt_plan.py +++ b/tests/test_adopt_plan.py @@ -17,7 +17,8 @@ import pytest from conftest import ADOPT, REPO, git, resolve, run_script, write_plan -from test_adopt_project_is_the_source import RENAME_FIRST +from test_adopt_project_is_the_source import (DERIVED_OWN_NAME, NAME_RULE, + RENAME_FIRST) sys.path.insert(0, str(REPO / "scripts")) from path_classify import PathPolicy # noqa: E402 @@ -380,15 +381,15 @@ def test_a_project_name_that_is_not_a_lowercase_id_is_refused(source_repo, DERIVED_ID = "--id (not given, so --project lowercased)" #: And what it is told to do (L2-54): change `--project`, which the naming #: policy reads itself, so "pass an explicit lowercase --id" led to -#: `naming-unclassified`. +#: `naming-unclassified`; and to what that refusal's rule takes, for +#: "change --project" alone led to `North-Wind`, which it refuses (rv699). CHANGE_PROJECT = ( - "Remediation: change --project. The id is --project lowercased, and an " - "explicit --id would not change --project, which also names the " - "assembly root and both legs.") + f"Remediation: change --project to {NAME_RULE}. The id is --project " + "lowercased, and an explicit --id would not change --project, which " + "also names the assembly root and both legs.") #: Or, where `--project` is the name of the repository the source names, -#: adopted in place, `_rename_first`'s words. -IN_PLACE = ("Remediation: the id is --project lowercased, and --project is " - f"the source's own name. testorg/North_Wind {RENAME_FIRST}") +#: adopted in place, `_rename_first`'s words after the same rule. +IN_PLACE = f"Remediation: {DERIVED_OWN_NAME} testorg/North_Wind {RENAME_FIRST}" def bad_id(result) -> list[str]: @@ -460,6 +461,24 @@ def test_an_id_derived_from_the_project_is_named_as_derived( assert not plan.exists() +def test_a_derived_ids_remedy_names_the_rule_naming_unclassified_does( + source_repo, tmp_path): + """rv699: "change --project" alone sent the person to `North-Wind`, which + `naming-unclassified` then refused. The remedy names the rule that + refusal gives for `--project`, in its own words, and the next refusal + of the same `--project` says the same rule.""" + derived = bad_id(write_plan(source_repo, tmp_path / "derived.yaml", + project="North_Wind")) + named = bad_id(write_plan(source_repo, tmp_path / "named.yaml", + project="North_Wind", + extra=("--id", "north-wind"))) + assert named == [ + "REFUSED naming-unclassified: 'North_Wind' matches no family in the " + "naming policy", f"Remediation: --project takes {NAME_RULE}."], named + assert derived[1].startswith( + f"Remediation: change --project to {NAME_RULE}. "), derived + + def test_a_passed_id_equal_to_the_derived_one_is_still_named_as_passed( source_repo, tmp_path): """Whether the id was passed is what the person did, not whether it diff --git a/tests/test_adopt_project_is_the_source.py b/tests/test_adopt_project_is_the_source.py index b1065873..219bc21b 100644 --- a/tests/test_adopt_project_is_the_source.py +++ b/tests/test_adopt_project_is_the_source.py @@ -55,6 +55,13 @@ "rename`); then run `plan` again against the renamed repository " "(`--source /`, or a clone whose `origin` names it), with " "its new name as `--project`.") +#: The rule `naming-unclassified` gives for `--project`, which a derived +#: id's remedy names too (#658, rv699). +NAME_RULE = "one CamelCase token with no hyphen, underscore, dot or space" +#: What `adopt-bad-id` says of an id derived from the source's own name, +#: before the repository and `RENAME_FIRST` (#658). +DERIVED_OWN_NAME = ("the id is --project lowercased, and --project is the " + f"source's own name, which must be {NAME_RULE}.") #: A source small enough that `plan` asks no question. TREE = {"README.md": "# here\n", "src/app.py": "print(1)\n", "specs/001/spec.md": "# Spec\n"} @@ -220,12 +227,9 @@ def test_a_source_that_names_no_repository_is_asked_nothing(tmp_path, assert load_yaml(tmp_path / PLAN)["source"]["repository"] is None -def test_an_org_repo_source_is_the_repository_it_names(adopter, tmp_path, - monkeypatch, capsys): - """`--source acme/Atlas` names the repository it clones, and is compared - as an `origin` is. `git clone` of a local repository stands in for `gh - repo clone`, and is the one command the run is let make.""" - local = source_of(tmp_path, None, "Atlas") +def cloned_from(adopter, monkeypatch, local: Path) -> None: + """`gh repo clone` replaced by `git clone` of the local repository + `local`, the one command the run is let make.""" real = adopter.run def cloned(args, *rest, **keywords): @@ -234,6 +238,14 @@ def cloned(args, *rest, **keywords): **keywords) monkeypatch.setattr(adopter, "run", cloned) + + +def test_an_org_repo_source_is_the_repository_it_names(adopter, tmp_path, + monkeypatch, capsys): + """`--source acme/Atlas` names the repository it clones, and is compared + as an `origin` is. `git clone` of a local repository stands in for `gh + repo clone`.""" + cloned_from(adopter, monkeypatch, source_of(tmp_path, None, "Atlas")) code = adopter.main(["plan", "--source", "acme/Atlas", "--project", "Atlsa", "--elected-by", "Test Human", "--elected-on", "2026-10-13", "--out", @@ -249,6 +261,28 @@ def cloned(args, *rest, **keywords): assert not (tmp_path / PLAN).exists() +def test_a_derived_id_of_a_cloned_sources_own_name_says_rename_first( + adopter, tmp_path, monkeypatch, capsys): + """#658, rv699: `--source testorg/North_Wind` names the repository it + clones as an `origin` does, so the id derived from that name is told to + rename it first, as for a local clone, and not to change `--project`.""" + cloned_from(adopter, monkeypatch, source_of(tmp_path, None, "North_Wind")) + code = adopter.main(["plan", "--source", "testorg/North_Wind", + "--project", "North_Wind", "--elected-by", + "Test Human", "--elected-on", "2026-10-13", "--out", + str(tmp_path / PLAN), "--work-dir", + str(tmp_path / "work")]) + + assert code == 2 + err = capsys.readouterr().err + assert err.splitlines()[:2] == [ + "REFUSED adopt-bad-id: --id (not given, so --project lowercased) is " + "'north_wind', which must match ^[a-z0-9][a-z0-9-]*$", + f"Remediation: {DERIVED_OWN_NAME} testorg/North_Wind {RENAME_FIRST}" + ], err + assert not (tmp_path / PLAN).exists() + + # --- `check` and `execute` -------------------------------------------------- @pytest.mark.parametrize("changes, said", [ diff --git a/tests/test_naming_policy.py b/tests/test_naming_policy.py index c229287c..99bb249b 100644 --- a/tests/test_naming_policy.py +++ b/tests/test_naming_policy.py @@ -5,6 +5,7 @@ import errno import os +import string import sys from pathlib import Path @@ -17,8 +18,9 @@ sys.path.insert(0, str(REPO / "scripts")) from repo_shape import ( # noqa: E402 - UNAMBIGUOUS_FORMS, LinkPins, NamingPolicy, Refusal, accepts_role, - link_manifest_lookup, link_pins_from_trees, parse_yaml, + PROJECT_ID_RE, SAFE_ARG_RE, UNAMBIGUOUS_FORMS, LinkPins, NamingPolicy, + Refusal, accepts_role, link_manifest_lookup, link_pins_from_trees, + parse_yaml, ) from shape_materialize import naming_block # noqa: E402 @@ -224,6 +226,34 @@ def test_unclassified_examples_match_no_family(policy): assert policy.classify(name) is None, f"{name} unexpectedly classified" +#: What a `--project` or a `--family` can hold past `checked_value` +#: (`SAFE_ARG_RE`) that no id takes once lowercased (`PROJECT_ID_RE`): the +#: characters an id derived from the name is refused for. +NO_ID_CHARACTERS = (".", "_", "/", "@", "+", "~") + + +def test_those_are_the_characters_the_two_patterns_disagree_on(): + """`NO_ID_CHARACTERS` is every character the two patterns disagree on, + so one either of them gains or loses is asked about below.""" + between = sorted(c for c in string.printable + if SAFE_ARG_RE.fullmatch(f"a{c}b") + and not PROJECT_ID_RE.match(f"a{c}b".lower())) + assert between == sorted(NO_ID_CHARACTERS) + + +@pytest.mark.parametrize("character", NO_ID_CHARACTERS) +def test_no_root_or_family_name_holds_a_character_no_id_takes(policy, + character): + """#658 and #659 tell a derived id refused for one of these characters + to change `--project` or `--family` to one CamelCase token, never to + pass an `--id`, and, in place, that the source cannot be adopted under + that name. That holds while the policy classifies no such name as an + assembly root or a family holder.""" + name = f"North{character}Wind" + assert policy.classify(name, "assembly", set()) is None, name + assert policy.classify(name, "family") is None, name + + def test_overlaps_are_reported_not_hidden(policy): """`openxFactory` satisfies three forms; precedence picks one, --explain shows all three."""