mirror of
https://github.com/kuhyx/testsAndMisc.git
synced 2026-11-10 18:47:04 +01:00
extract_shell_functions: recognise the function name() spelling
The detection regex matched only `name() {`. bash also accepts
`function name() {` and `function name {`, and pacman_wrapper.sh uses the
keyword form for 17 of its 30 functions — so a split of that file would have
left every one of them behind in the entry script while the tool reported
success. Silent, and exactly the kind of half-done split the verifier is
meant to catch afterwards rather than the extractor causing.
Parentheses stay mandatory in the non-keyword branch. Making them optional
everywhere also matches brace groups and control constructs, which would
sweep non-function code into the library — the opposite failure, and worse.
Nine negative tests pin that boundary.
The function name now comes from a named capture group rather than
splitting on "(", which returned "function name" for the keyword form and
would have broken both --functions selection and the verifier's name matching.
Verified against the already-split files first: cco_state.sh still yields 13
functions and leechblock_fetch.sh 3, so the completed splits are unaffected.
pacman_wrapper.sh goes from 13 detected to 31.
First tests for this script: 20 cases covering every accepted spelling,
brace matching across nested bodies, and single-line functions.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01F3PzGc75gBcdxP3PkHXGcV
This commit is contained in:
parent
ddd3738976
commit
12e9f5916f
@ -0,0 +1,36 @@
|
||||
{
|
||||
"intent": "Teach extract_shell_functions.py bash's `function name() {` spelling. It matched only `name() {`, so a file using the keyword form had those functions silently left behind while the tool still reported success.",
|
||||
"scope": [
|
||||
"meta/scripts/extract_shell_functions.py: the function-detection regex and how the name is derived",
|
||||
"python_pkg/tests/test_extract_shell_functions.py: first tests for this script",
|
||||
"Non-goal: any change to how blocks are brace-matched or written out"
|
||||
],
|
||||
"changes": [
|
||||
"The regex now accepts `name() {`, `function name() {` and `function name {`. Parentheses stay MANDATORY in the non-keyword branch: making them optional everywhere would also match brace groups and control constructs, which would sweep non-function code into the library — the opposite failure and a worse one.",
|
||||
"The name comes from a named capture group instead of lines[start].split('(')[0], which yielded 'function name' for the keyword form and would have produced a corrupt --functions match and a wrong verify_shell_split.sh name."
|
||||
],
|
||||
"verification": [
|
||||
{
|
||||
"command": "python3 -m pytest python_pkg/tests/test_extract_shell_functions.py -q",
|
||||
"result": "pass",
|
||||
"evidence": "20 passed. Covers all five spellings bash accepts, nine negative cases (indented definitions, `while true; do {`, `if x {`, bare braces, a commented-out definition, a bare call), brace matching across a nested if/case body, single-line functions, and a file mixing all three forms."
|
||||
},
|
||||
{
|
||||
"command": "find_function_blocks on the real files",
|
||||
"result": "pass",
|
||||
"evidence": "pacman_wrapper.sh now yields 31 functions where the old pattern found 13 — the 17 `function`-keyword definitions plus one the count had missed. cco_state.sh (13) and leechblock_fetch.sh (3) are unchanged, so the already-completed splits would extract identically."
|
||||
},
|
||||
{
|
||||
"command": "pre-commit run --files <both files>",
|
||||
"result": "pass",
|
||||
"evidence": "ruff (2 import-order fixes applied), ruff format, mypy, pylint, bandit, pytest-with-coverage and codespell all pass. No suppression added."
|
||||
}
|
||||
],
|
||||
"risks": [
|
||||
"A file that legitimately contains a top-level brace group opening at column 0 would now be misread if it also began with an identifier. The mandatory-parens rule plus the nine negative tests are what bound this; the alternative (missing real functions) is the failure that actually occurred.",
|
||||
"This changes the tool every remaining split depends on. It was verified against the three already-split files first, so a regression would have shown up as a different function count there."
|
||||
],
|
||||
"rollback": [
|
||||
"git revert the commit; the extractor returns to matching only `name() {` and the new tests fail, which is the intended signal rather than a silent regression."
|
||||
]
|
||||
}
|
||||
@ -18,7 +18,19 @@ from pathlib import Path
|
||||
import re
|
||||
import sys
|
||||
|
||||
_FUNC_RE = re.compile(r"^[a-zA-Z_][a-zA-Z0-9_]*\(\)\s*\{")
|
||||
# Both bash spellings: `name() {` and `function name() {`. Accepting only the
|
||||
# first silently skipped 17 of the 30 functions in pacman_wrapper.sh, leaving
|
||||
# them in the entry script while reporting success -- a split that looks done
|
||||
# and is not.
|
||||
#
|
||||
# `()` is required unless the `function` keyword is present. Making the parens
|
||||
# optional in both branches would match plain brace groups and control blocks
|
||||
# (`while read -r x; do ... done {`-style constructs and `if ... {` in other
|
||||
# dialects), sweeping non-function code into the library.
|
||||
_FUNC_RE = re.compile(
|
||||
r"^(?:function\s+(?P<kwname>[a-zA-Z_][a-zA-Z0-9_]*)\s*(?:\(\))?"
|
||||
r"|(?P<name>[a-zA-Z_][a-zA-Z0-9_]*)\s*\(\))\s*\{"
|
||||
)
|
||||
|
||||
|
||||
def find_function_blocks(lines: list[str]) -> list[tuple[int, int, str]]:
|
||||
@ -26,7 +38,11 @@ def find_function_blocks(lines: list[str]) -> list[tuple[int, int, str]]:
|
||||
blocks: list[tuple[int, int, str]] = []
|
||||
index = 0
|
||||
while index < len(lines):
|
||||
if _FUNC_RE.match(lines[index]):
|
||||
match = _FUNC_RE.match(lines[index])
|
||||
if match:
|
||||
# Take the name from the regex, not from splitting on "(": the
|
||||
# `function name()` form would otherwise yield "function name".
|
||||
name = match.group("kwname") or match.group("name")
|
||||
start, depth = index, 0
|
||||
while index < len(lines):
|
||||
depth += lines[index].count("{") - lines[index].count("}")
|
||||
@ -35,7 +51,7 @@ def find_function_blocks(lines: list[str]) -> list[tuple[int, int, str]]:
|
||||
if depth <= 0:
|
||||
break
|
||||
index += 1
|
||||
blocks.append((start, index, lines[start].split("(")[0]))
|
||||
blocks.append((start, index, name))
|
||||
index += 1
|
||||
return blocks
|
||||
|
||||
|
||||
140
python_pkg/tests/test_extract_shell_functions.py
Normal file
140
python_pkg/tests/test_extract_shell_functions.py
Normal file
@ -0,0 +1,140 @@
|
||||
"""Tests for meta/scripts/extract_shell_functions.py.
|
||||
|
||||
The extractor is what every 250-line-cap split runs through, and its failure
|
||||
mode is silent: a function form it does not recognise is simply left behind in
|
||||
the entry script while the tool still reports success. That is how 17 of the
|
||||
30 functions in pacman_wrapper.sh were nearly missed — the original pattern
|
||||
matched only `name() {` and not bash's `function name() {` spelling.
|
||||
"""
|
||||
|
||||
from __future__ import annotations
|
||||
|
||||
import importlib.util
|
||||
from pathlib import Path
|
||||
from typing import TYPE_CHECKING
|
||||
|
||||
import pytest
|
||||
|
||||
if TYPE_CHECKING:
|
||||
from types import ModuleType
|
||||
|
||||
_ROOT = Path(__file__).resolve().parents[2]
|
||||
_SCRIPT = _ROOT / "meta" / "scripts" / "extract_shell_functions.py"
|
||||
|
||||
|
||||
def _load() -> ModuleType:
|
||||
"""Import the script by path; it lives outside any importable package."""
|
||||
spec = importlib.util.spec_from_file_location("extract_shell_functions", _SCRIPT)
|
||||
assert spec is not None
|
||||
assert spec.loader is not None
|
||||
module = importlib.util.module_from_spec(spec)
|
||||
spec.loader.exec_module(module)
|
||||
return module
|
||||
|
||||
|
||||
@pytest.fixture(name="esf", scope="module")
|
||||
def _esf() -> ModuleType:
|
||||
return _load()
|
||||
|
||||
|
||||
@pytest.mark.parametrize(
|
||||
("line", "expected"),
|
||||
[
|
||||
# Both spellings bash accepts, with and without whitespace variants.
|
||||
("foo() {", "foo"),
|
||||
("foo(){", "foo"),
|
||||
("foo () {", "foo"),
|
||||
("function foo() {", "foo"),
|
||||
("function foo(){", "foo"),
|
||||
# The keyword form may omit the parentheses entirely.
|
||||
("function foo {", "foo"),
|
||||
("_leading_underscore() {", "_leading_underscore"),
|
||||
("name2() {", "name2"),
|
||||
],
|
||||
)
|
||||
def test_recognises_every_function_spelling(
|
||||
esf: ModuleType, line: str, expected: str
|
||||
) -> None:
|
||||
"""Every form bash accepts must be detected, and named correctly."""
|
||||
match = esf._FUNC_RE.match(line)
|
||||
assert match is not None, f"{line!r} was not recognised as a function"
|
||||
name = match.group("kwname") or match.group("name")
|
||||
assert name == expected
|
||||
|
||||
|
||||
@pytest.mark.parametrize(
|
||||
"line",
|
||||
[
|
||||
# Indented definitions are not top level.
|
||||
" indented() {",
|
||||
"\tindented() {",
|
||||
# Brace groups and control flow must never be swept into a library:
|
||||
# without the mandatory (), these would match and the extractor would
|
||||
# move non-function code.
|
||||
"while true; do {",
|
||||
"if x {",
|
||||
"{",
|
||||
"}",
|
||||
# A commented-out definition is not a definition.
|
||||
"# foo() {",
|
||||
# A call is not a definition.
|
||||
"foo()",
|
||||
"",
|
||||
],
|
||||
)
|
||||
def test_ignores_everything_that_is_not_a_top_level_function(
|
||||
esf: ModuleType, line: str
|
||||
) -> None:
|
||||
assert esf._FUNC_RE.match(line) is None, (
|
||||
f"{line!r} was wrongly treated as a function"
|
||||
)
|
||||
|
||||
|
||||
def test_brace_matching_spans_the_whole_body(esf: ModuleType) -> None:
|
||||
"""A function block must end at its own closing brace, not the first one."""
|
||||
lines = [
|
||||
"function outer() {",
|
||||
"\tif true; then",
|
||||
"\t\techo nested",
|
||||
"\tfi",
|
||||
"\tcase $x in",
|
||||
"\ta) echo a ;;",
|
||||
"\tesac",
|
||||
"}",
|
||||
"",
|
||||
"after() {",
|
||||
"\techo after",
|
||||
"}",
|
||||
]
|
||||
blocks = esf.find_function_blocks(lines)
|
||||
assert [name for _, _, name in blocks] == ["outer", "after"]
|
||||
start, end, _ = blocks[0]
|
||||
assert (start, end) == (0, 7)
|
||||
|
||||
|
||||
def test_single_line_function_closes_on_its_own_line(esf: ModuleType) -> None:
|
||||
"""`name() { ...; }` opens and closes on one line."""
|
||||
blocks = esf.find_function_blocks(["logg() { printf '%s\\n' \"$*\"; }", "echo hi"])
|
||||
assert [name for _, _, name in blocks] == ["logg"]
|
||||
start, end, _ = blocks[0]
|
||||
assert start == end == 0
|
||||
|
||||
|
||||
def test_detects_both_spellings_in_one_file(esf: ModuleType) -> None:
|
||||
"""The regression that motivated these tests: a file mixing both forms."""
|
||||
lines = [
|
||||
"#!/bin/bash",
|
||||
"plain() {",
|
||||
"\techo plain",
|
||||
"}",
|
||||
"",
|
||||
"function keyword() {",
|
||||
"\techo keyword",
|
||||
"}",
|
||||
"",
|
||||
"function no_parens {",
|
||||
"\techo no_parens",
|
||||
"}",
|
||||
]
|
||||
names = [name for _, _, name in esf.find_function_blocks(lines)]
|
||||
assert names == ["plain", "keyword", "no_parens"]
|
||||
Loading…
Reference in New Issue
Block a user