diff --git a/scaffold-project.py b/scaffold-project.py index 08444981..51ead0ed 100755 --- a/scaffold-project.py +++ b/scaffold-project.py @@ -703,7 +703,7 @@ def _refuse_unholdable_manifest(args, values: dict, policy) -> None: """ flags = dict(MANIFEST_FLAGS) for key, flag, given, source in ( - ("PROJECT_ID", "--id", args.id, "--project lowercased"), + ("PROJECT_ID", "--id", args.id, ID_DERIVED_FROM), ("PROJECT_NAME", "--name", args.name, "--project"), ("ELECTED_BY", "--elected-by", args.elected_by, "GIT_AUTHOR_NAME or `git config user.name`")): @@ -774,6 +774,58 @@ def _identity(args, project: str) -> tuple[str, str, str, str, str]: return project_id, display, elected_on, reference, elected_by +#: Where `_identity` takes the id from when no `--id` is passed, as a +#: sentence names it: in the label `_refuse_unholdable_manifest` gives a +#: value no flag supplied, and in `scaffold-bad-id` (#659). +ID_DERIVED_FROM = "--project lowercased" + +#: The rule a `--project` is held to, as `naming-unclassified` states it +#: (`_check_one_name`). `scaffold-bad-id`'s derived remedy names it in the +#: same words, so the person who follows it is not sent from one refusal to +#: the other (#659). +PROJECT_NAME_RULE = ("one CamelCase token with no hyphen, underscore, dot " + "or space") + +#: How `scaffold-bad-id` is told to go on (#659), in the words +#: `adopt-project.py plan` gives `adopt-bad-id` (#658). A `--id` the person +#: passed is theirs to retype. An id `_identity` derived is `--project` +#: lowercased, and `--project` also names the three repositories, which the +#: naming policy reads itself: every form it holds takes only ASCII letters, +#: digits and hyphens, so "pass an explicit lowercase --id" led straight to +#: `naming-unclassified`, and so would "change --project" to `North-Wind`. +#: The derived remedy names the rule. +PASSED_ID_REMEDIATION = "Remediation: pass an explicit lowercase --id." +DERIVED_ID_REMEDIATION = ( + f"Remediation: change --project to {PROJECT_NAME_RULE}. The id is " + f"{ID_DERIVED_FROM}, and an explicit --id would not change --project, " + "which also names the assembly root and both legs.") + + +def _refuse_bad_id(project_id: str, given: str | None) -> None: + """`scaffold-bad-id`: an id the project's validators would not take + (`PROJECT_ID_RE`), named by where it came from (#659), as + `adopt-project.py plan` names `adopt-bad-id` (#658). + + `given` is `--id` as parsed. One the person passed is named as the flag + they typed, value as typed, and keeps its remedy. With none, or an empty + one, `_identity` derived the id (`args.id or project.lower()`), and + `--id 'north_wind' must match` named a value nobody had typed: the flag + is named as `_refuse_unholdable_manifest` names one, by where its value + came from, and an empty `--id ''` as empty, because it was passed.""" + if PROJECT_ID_RE.match(project_id): + return + pattern = PROJECT_ID_RE.pattern + if given: + raise Refusal("scaffold-bad-id", + f"--id {project_id!r} must match {pattern}", + PASSED_ID_REMEDIATION) + unset = "not given" if given is None else "empty" + raise Refusal("scaffold-bad-id", + f"--id ({unset}, so {ID_DERIVED_FROM}) is {project_id!r}, " + f"which must match {pattern}", + DERIVED_ID_REMEDIATION) + + #: How a refusal names an elector `_elector` did not get from `--elected-by` #: (#282), so the sentence says where the value came from and not a flag #: nobody typed: the words #255's `plan` uses for its own fallback. @@ -1080,8 +1132,7 @@ def _check_one_name(policy, name: str, role: str, declared_pins: set, raise Refusal( "naming-unclassified", f"{name!r} matches no family in the naming policy. " - "`` is one CamelCase token with no hyphen, " - "underscore, dot or space.", + f"`` is {PROJECT_NAME_RULE}.", "Remediation: re-run with a --project value of that form.", ) if not accepts_role(found, role): @@ -1774,10 +1825,7 @@ def _scaffold(args) -> int: local, repositories, urls = _repository_urls(args, names) _refuse_an_unwritable_clone_url(local, urls["assembly"]) policy = NamingPolicy.load(SHAPE_ROOT / "contracts" / "repository-naming.yaml") - if not PROJECT_ID_RE.match(project_id): - raise Refusal("scaffold-bad-id", - f"--id {project_id!r} must match {PROJECT_ID_RE.pattern}", - "Remediation: pass an explicit lowercase --id.") + _refuse_bad_id(project_id, args.id) # WHAT THE PROJECT DECLARES ABOUT ITSELF, before any name is judged. # With no `--pin`, a descendant-form name is a claim with no referent and # the DECLARED ROLE wins. With `--pin openGlass@`, `MedxGlass` IS a diff --git a/scripts/family.py b/scripts/family.py index e4f430cb..40d59442 100755 --- a/scripts/family.py +++ b/scripts/family.py @@ -585,6 +585,14 @@ def _member_url(repository: str, local_remote_dir: Path | None) -> str: f"{repo_basename(repository)}.git")) +#: The rule a `--family` is held to, as `naming-unclassified` states it +#: (`_classify_family_name`). `family-bad-id`'s derived remedy names it in +#: the same words, so the person who follows it is not sent from one +#: refusal to the other (#659). +FAMILY_NAME_RULE = ("one CamelCase token with no hyphen, underscore, dot " + "or space") + + def _classify_family_name(policy: NamingPolicy, name: str) -> None: """The family name against the policy, in the form a family declares. @@ -598,9 +606,9 @@ def _classify_family_name(policy: NamingPolicy, name: str) -> None: raise Refusal( "naming-unclassified", f"{name!r} matches no family in the naming policy", - "Remediation: --family takes one CamelCase token with no hyphen, " - "underscore, dot or space — the same rule an assembly root's name " - "follows, because a holder is spelled like one.") + f"Remediation: --family takes {FAMILY_NAME_RULE} — the same rule " + "an assembly root's name follows, because a holder is spelled " + "like one.") if found.family != "family": raise Refusal( "naming-not-a-family", @@ -1502,6 +1510,53 @@ def _landing(args, family: str) -> tuple[Path | None, Path]: return folder, holder +#: Where `init` takes the id from when no `--id` is passed, as a sentence +#: names it: in the label `_manifest_flags` gives a value no flag supplied, +#: and in `family-bad-id` (#659). +FAMILY_ID_DERIVED_FROM = "--family lowercased" + +#: How `family-bad-id` is told to go on (#659), in the words the scaffold +#: gives `scaffold-bad-id` and `adopt-project.py plan` `adopt-bad-id` +#: (#658). A `--id` the person passed is theirs to retype. An id `init` +#: derived is `--family` lowercased, and `--family` also names the holder +#: and, unless `--work-dir` is given, its family folder (`_landing`); the +#: name is the one `_classify_family_name` reads against a policy whose +#: forms take only ASCII letters, digits and hyphens, so "pass an explicit +#: lowercase --id" led straight to `naming-unclassified`, and so would +#: "change --family" to `North-Wind`. The derived remedy names the rule. +PASSED_FAMILY_ID_REMEDIATION = "Remediation: pass an explicit lowercase --id." +DERIVED_FAMILY_ID_REMEDIATION = ( + f"Remediation: change --family to {FAMILY_NAME_RULE}. The id is " + f"{FAMILY_ID_DERIVED_FROM}, and an explicit --id would not change " + "--family, which also names the holder and, unless --work-dir is given, " + "its family folder.") + + +def _refuse_bad_family_id(family_id: str, given: str | None) -> None: + """`family-bad-id`: an id `validate-family.py` would not take + (`PROJECT_ID_RE`), named by where it came from (#659), as the scaffold + names `scaffold-bad-id`. + + `given` is `--id` as parsed. One the person passed is named as the flag + they typed, value as typed, and keeps its remedy. With none, or an empty + one, the id is `--family` lowercased (`args.id or family.lower()`), and + `--id 'north_wind' must match` named a value nobody had typed: the flag + is named as `_manifest_flags` names one, by where its value came from, + and an empty `--id ''` as empty, because it was passed.""" + if PROJECT_ID_RE.match(family_id): + return + pattern = PROJECT_ID_RE.pattern + if given: + raise Refusal("family-bad-id", + f"--id {family_id!r} must match {pattern}", + PASSED_FAMILY_ID_REMEDIATION) + unset = "not given" if given is None else "empty" + raise Refusal("family-bad-id", + f"--id ({unset}, so {FAMILY_ID_DERIVED_FROM}) is " + f"{family_id!r}, which must match {pattern}", + DERIVED_FAMILY_ID_REMEDIATION) + + def _checked_family_identity(args) -> tuple[str, str, str]: """`(family, family_id, topic)` — every name check `init` makes before it decides where anything lands. @@ -1523,10 +1578,7 @@ def _checked_family_identity(args) -> tuple[str, str, str]: refuse_bad_branch("family-bad-tracking-branch", "--tracking-branch", args.tracking_branch) family_id = checked_value("--id", args.id or family.lower()) - if not PROJECT_ID_RE.match(family_id): - raise Refusal("family-bad-id", - f"--id {family_id!r} must match {PROJECT_ID_RE.pattern}", - "Remediation: pass an explicit lowercase --id.") + _refuse_bad_family_id(family_id, args.id) policy = NamingPolicy.load(NAMING_POLICY) _classify_family_name(policy, family) topic = policy.topic_for(family_id) @@ -2332,7 +2384,7 @@ def _manifest_flags(args) -> dict: from, so that a refusal does not point at a flag nobody typed.""" flags = dict(MANIFEST_FLAGS) for key, flag, given, source in ( - ("FAMILY_ID", "--id", args.id, "--family lowercased"), + ("FAMILY_ID", "--id", args.id, FAMILY_ID_DERIVED_FROM), ("FAMILY_NAME", "--name", args.name, "--family"), ("CREATED_BY", "--created-by", args.created_by, "GIT_AUTHOR_NAME or `git config user.name`"), diff --git a/tests/test_derived_id_refusals.py b/tests/test_derived_id_refusals.py new file mode 100644 index 00000000..a2eb0883 --- /dev/null +++ b/tests/test_derived_id_refusals.py @@ -0,0 +1,174 @@ +# SPDX-License-Identifier: Apache-2.0 +"""`scaffold-bad-id` and `family-bad-id` name an id by where it came from, +and send a derived one to the flag it came from (#659). + +WHAT WAS WRONG (#459's L2-56, and its label half in comment 6198174555). +`scaffold-project.py` fills `--id` in as `--project` lowercased when none is +passed, and `family.py init` as `--family` lowercased. Both then refused that +id as `--id 'north_wind' must match ^[a-z0-9][a-z0-9-]*$`, a flag nobody had +passed, with "Remediation: pass an explicit lowercase --id." And that remedy +led straight to a second refusal: the naming policy reads `--project` +(`--family`) itself, every form it holds takes only ASCII letters, digits +and hyphens, so `--id north-wind` met `naming-unclassified: 'North_Wind' +matches no family in the naming policy`. + +THE CURE, in the words `adopt-project.py plan` uses for `adopt-bad-id` +(#658): a derived id's flag is named by where its value came from, `--id +(not given, so --project lowercased)`, the label each tool's manifest +refusals already give it, and its remedy is to change the flag it came from, +which also names what the run makes. A `--id` the person passed keeps +today's sentence and remedy, value as typed, even when it equals the derived +one: what is read is whether the flag was passed, not what its value is. An +empty `--id ''` derives the id as no `--id` does (`args.id or ...`), and is +named as empty, because it was passed. + +Every run is a `--dry-run` into `--local-remote-dir`, which never calls `gh`. +""" + +from __future__ import annotations + +import sys + +import pytest + +from conftest import ORG, REPO, SCAFFOLD, run_script + +sys.path.insert(0, str(REPO / "scripts")) +from repo_shape import PROJECT_ID_RE, SAFE_ARG_RE # noqa: E402 + +FAMILY = REPO / "scripts" / "family.py" +PATTERN = PROJECT_ID_RE.pattern + +#: The rule each tool's `naming-unclassified` holds a name to, in its own +#: words: the scaffold's and `init`'s are the same. +NAME_RULE = "one CamelCase token with no hyphen, underscore, dot or space" + +#: Per tool: the command, the flag naming whoever creates it, the flag the +#: id is derived from and what else that flag names, and the finding's code. +TOOLS = { + "scaffold": {"command": (SCAFFOLD,), "who": "--elected-by", + "flag": "--project", "code": "scaffold-bad-id", + "names": "the assembly root and both legs"}, + "family": {"command": (FAMILY, "init"), "who": "--created-by", + "flag": "--family", "code": "family-bad-id", + "names": "the holder and, unless --work-dir is given, its " + "family folder"}, +} +PASSED_REMEDY = "Remediation: pass an explicit lowercase --id." + + +def derived(tool: str, unset: str) -> str: + """How `tool` names a derived id's flag: `--id` given no value + (`unset`), so the flag the id is derived from, lowercased.""" + return f"--id ({unset}, so {TOOLS[tool]['flag']} lowercased)" + + +def derived_remedy(tool: str) -> str: + """The remedy `tool` gives for an id derived from its flag: change the + flag, to a name the naming policy takes.""" + flag = TOOLS[tool]["flag"] + return (f"Remediation: change {flag} to {NAME_RULE}. The id is {flag} " + f"lowercased, and an explicit --id would not change {flag}, " + f"which also names {TOOLS[tool]['names']}.") + + +#: Every character a name may hold on a command line (`SAFE_ARG_RE`, which +#: `checked_value` asks of `--project` and `--family` before the id is +#: derived from it) that leaves the name, lowercased, no id +#: (`PROJECT_ID_RE`): the characters a DERIVED id is refused for. +REFUSED_IN_AN_ID = tuple( + c for c in map(chr, range(0x21, 0x7F)) + if SAFE_ARG_RE.fullmatch(f"North{c}Wind") + and not PROJECT_ID_RE.match(f"North{c}Wind".lower())) + + +def run_tool(tmp_path, tool: str, name: str, *extra: str): + """One dry run of `tool` for `name`, into a local remote that never + exists: every row here is refused before anything is made.""" + remotes = tmp_path / "remotes" + result = run_script(*TOOLS[tool]["command"], "--org", ORG, + TOOLS[tool]["flag"], name, + TOOLS[tool]["who"], "Test Human", "--dry-run", + "--local-remote-dir", str(remotes), + "--work-dir", str(tmp_path / "work"), *extra) + assert result.returncode == 2, result.stdout + result.stderr + assert not remotes.exists() + return result.stderr + + +def refusal(tool: str, what: str, remedy: str) -> str: + return f"REFUSED {TOOLS[tool]['code']}: {what}\n{remedy}\n" + + +def assert_named_as_passed(tool: str, stderr: str, value: str) -> None: + """A passed `--id` gets none of a derived id's words, neither its label + ("not given") nor its remedy ("change --project"), and keeps today's + sentence and remedy, value as typed. The derived words are asked first, + so a run that gives a passed id either of them fails on that word.""" + assert "not given" not in stderr + assert f"change {TOOLS[tool]['flag']}" not in stderr + assert refusal(tool, f"--id {value!r} must match {PATTERN}", + PASSED_REMEDY) in stderr + + +@pytest.mark.parametrize("tool", TOOLS) +def test_a_derived_id_is_named_by_where_it_came_from_and_sent_to_that_flag( + tmp_path, tool): + stderr = run_tool(tmp_path, tool, "North_Wind") + assert refusal( + tool, + f"{derived(tool, 'not given')} is 'north_wind', which must match " + f"{PATTERN}", + derived_remedy(tool)) in stderr + assert "--id 'north_wind'" not in stderr + assert PASSED_REMEDY not in stderr + + +@pytest.mark.parametrize("tool", TOOLS) +def test_an_empty_id_is_named_as_empty_and_not_as_not_given(tmp_path, tool): + """`--id ''` was passed, so "not given" would be false of it; the id is + still the derived one, and the remedy is still the flag's.""" + stderr = run_tool(tmp_path, tool, "North_Wind", "--id=") + assert refusal( + tool, + f"{derived(tool, 'empty')} is 'north_wind', which must match " + f"{PATTERN}", + derived_remedy(tool)) in stderr + assert "not given" not in stderr + + +@pytest.mark.parametrize("tool", TOOLS) +def test_a_passed_id_equal_to_the_derived_one_is_still_named_as_passed( + tmp_path, tool): + """Passed means typed, not "differs from the name lowercased".""" + stderr = run_tool(tmp_path, tool, "North_Wind", "--id", "north_wind") + assert_named_as_passed(tool, stderr, "north_wind") + + +@pytest.mark.parametrize("tool", TOOLS) +def test_a_passed_bad_id_keeps_its_sentence_and_its_remedy(tmp_path, tool): + stderr = run_tool(tmp_path, tool, "NorthWind", "--id", "Foo_Bar") + assert_named_as_passed(tool, stderr, "Foo_Bar") + + +def test_the_characters_a_derived_id_is_refused_for(): + """The rows below are every one of them; an empty list would pass + vacuously, and a new one is a name the remedy has to be true of.""" + assert REFUSED_IN_AN_ID == ("+", ".", "/", "@", "_", "~") + + +@pytest.mark.parametrize("tool", TOOLS) +@pytest.mark.parametrize("char", REFUSED_IN_AN_ID) +def test_a_name_whose_derived_id_is_refused_is_refused_by_the_naming_policy( + tmp_path, tool, char): + """Why a derived id's remedy is its flag's and not `--id`'s: the naming + policy reads the name itself and refuses every one whose lowercase is no + id, so a `--id` that is an id only meets that refusal. Run with one, so + this is the run the old remedy led to. And the refusal states the rule + the derived remedy names, in the same words.""" + name = f"North{char}Wind" + stderr = run_tool(tmp_path, tool, name, "--id", "north-wind") + assert (f"REFUSED naming-unclassified: {name!r} matches no family in the " + "naming policy") in stderr + assert NAME_RULE in stderr + assert TOOLS[tool]["code"] not in stderr