Skip to content

chore: clean-up - #15566

Merged
kgryte merged 3 commits into
developfrom
philipp/fix-propagation-2026-09-26
Sep 26, 2026
Merged

kgryte merged 3 commits into
developfrom
philipp/fix-propagation-2026-09-26

Conversation

@Planeshifter

Copy link
Copy Markdown
Member

Description

What is the purpose of this pull request?

Propagating fixes merged to develop between 2026-09-25 13:09 UTC (947861b6) and 2026-09-26 13:09 UTC (05ed6e856) to sibling packages carrying the same defect.

Pattern: nested function declaration not depending on its immediate enclosing function

Commit f22d513b resolved a stdlib/no-unnecessary-nested-functions violation by dedenting a nested onRender declaration one scope level in _tools/docs/www/readme-database/lib/render.js; because that rule is enabled at severity error for source files (etc/eslint/rules/stdlib.js:4465) and the JavaScript lint workflow samples a rotating subset of files per run, every remaining violation is a latent lint failure. This PR applies the same one-level dedent at the twelve remaining sites whose flagged declaration only depends on bindings in a grandparent function scope. In each case, the JSDoc block moves with the declaration and the function body is unchanged apart from one tab of indentation.

  • f22d513b (chore: fix JavaScript lint errors)
    • _tools/github/{get,rank-followers,rank-users,user-details}: dedent done in lib/factory.js.
    • repl: dedent clbk in lib/commands/load.js.
    • repl/presentation: dedent onDrain in lib/commands/{first_fragment,next_slide,prev,prev_slide,redraw,reload_presentation,run_slide}.js.

Related Issues

Does this pull request have any related issues?

No.

Questions

Any questions for reviewers of this pull request?

Yes — one, concerning the rule rather than this diff.

stdlib/no-unnecessary-nested-functions emits "should be moved to outer function scope" for declarations whose only legal destination is module scope. stdlib lints with sourceType: 'script' plus env.node/env.commonjs (etc/eslint/parser-options/source_type.js, etc/eslint/env/index.js), which sets ecmaFeatures.globalReturn, so eslint-scope wraps the module body in a scope of type function. The rule skips a dependency only when depScope.type is 'global' or 'module' (lib/main.js), so a nested function whose sole free identifiers are module-level vars is reported as an outer-function-scope violation. Eight of the twenty candidates in this sweep are mislabeled this way. Worth a separate fix to the rule.

Other

Any other information relevant to this pull request? This may include screenshots, references, and/or implementation notes.

Validation

  • Search scope. The pattern signature is structural rather than textual, so the rule module was loaded into an ESLint Linter instance and swept over all 22,377 files matching lib/node_modules/@stdlib/**/lib/**/*.js. The harness was validated by reproducing CI's exact report against the pre-fix blob of render.js (82:3). **/benchmark/*.js, **/examples/*.js, **/test/*.js and *.md are excluded, per etc/eslint/overrides/index.js. The sweep found 29 violations: 20 reported as "outer function scope", 9 as "module scope".
  • Two independent Opus validation agents read each of the 20 candidate files in full, resolved each flagged function's free identifiers to their binding scopes via ESLint's scope manager, and simulated the dedent in memory before re-linting. Both returned the same 12 confirmations and the same 8 rejections, working without sight of each other's output.
  • Adaptation pass produced each per-site patch and verified it individually: violation gone, no new violation of the rule in the file, node --check clean.
  • Style-consistency pass compared every patch against the merged f22d513b shape and docs/style-guides/javascript, then applied all 12 together in a disposable worktree and re-ran the rule. All 12 approved without rewrite.
  • Post-apply checks. Whole-tree re-sweep: 29 violations before, 17 after. For every touched file, the multiset of trimmed non-empty lines is identical before and after, confirming that only indentation and brace placement changed and no code was added, removed, or rewritten.
  • stdlib's own pre-commit static analysis could not be run locally (no git hooks installed and no dev dependencies present in this environment), and stdlib's eslintrc-format ruleset cannot be loaded by the ESLint available here, so the patches are verified against the target rule rather than the full project ruleset. Their shape is identical to the already-reviewed f22d513b.

Deliberately excluded:

  • The 9 "module scope" violations. Remediation lifts a @private helper out of the exported function entirely, above // MAIN //, which requires introducing a // FUNCTIONS // section; a different and larger restructuring than the source commit's.
  • _tools/browserify/string (onDelete), iter/empty (factory), net/simple-http-server (onClose) and plot/sparklines/base/ctor (onRender). Reported as outer-function-scope violations but mislabeled per the question above: each is a direct child of a top-level function whose only free identifiers are module-level vars, so the fix is a module-scope hoist. Behavior-preserving, but out of this pattern's shape; none of the four files has a // FUNCTIONS // section.
  • array/base/{entries2views,nested2views,rekey-views,zip2views} (Datum). Hard rejections: each call decorates Datum.prototype with call-specific accessors via setNonEnumerableReadOnly/setReadWriteAccessor, which define properties as non-configurable. Hoisting gives every call one shared prototype, so the second invocation throws TypeError: Cannot redefine property. Both validation agents demonstrated the throw.
  • 14b80fb6 (docs: fix examples, wrapping dtype( x ) in String( … ) in README usage snippets): 19 matching sites across 11 files, all dropped. @stdlib/ndarray/dtype and @stdlib/array/dtype return primitive strings, so the wrapper is a runtime no-op; the stdlib/doctest rule executes the snippet and compares values, so it passes either way, and no style guide requires it.
  • 69c38fa9 (test: avoid out-of-memory error): no sibling uses cartesianPower( dtypes(), N ) with N >= 5. The nearest neighbour, quaternary-tiling-block-size at N = 4, does not exhaust memory, and applying the rotation would cut coverage from 65,536 combinations to 16 — a coverage-for-cost trade for a maintainer, not a defect fix.
  • f9e32db7 (test: increase tolerance to account for compiler optimizations): 173 textual matches for isAlmostSameValue( y, expected[ i ], 0 ) under stats/base/dists/**, none propagated. Zero ULPs is correct wherever the expression does not contract, the appropriate tolerance is per-formula, and confirming a site requires an FMA-enabled native build.
  • fa7647de (chore: clean-up): per-plugin JSDoc prose polish and a runtime-no-op array-construction change in private dev tooling. No defect at any sibling.
  • 836f90b9, 62cbbac0 and 947861b6: pattern searches returned zero remaining sites; each source commit already covered every occurrence.
  • 5a64ba79, 4c05b095, ea33a97f, dc56a799 and 77e92e53: docs: updates to auto-populated namespace tables of contents, related-packages sections and equation SVGs. Generator-owned — the fix belongs in the generator.
  • The seven test: migrate … to ULP-based assertions commits and every feat: commit in the window: not fix patterns.

Checklist

Please ensure the following tasks are completed before submitting this pull request.

AI Assistance

When authoring the changes proposed in this PR, did you use any kind of AI assistance?

  • Yes
  • No

If you answered "yes" above, how did you use AI assistance?

  • Code generation (e.g., when writing an implementation or fixing a bug)
  • Test/benchmark generation
  • Documentation (including examples)
  • Research and understanding

Disclosure

This PR was authored by Claude Code on behalf of @Planeshifter as an automated propagation of fixes merged to develop over the prior 24 hours. Candidate source commits were filtered for generalizable patterns, sibling sites located via a runnable pattern signature, and each proposed patch independently validated by parallel reviewer agents before commits were applied in the primary worktree. A maintainer should audit and promote out of draft.



Generated by Claude Code

Propagates fix from f22d513 ("chore: fix JavaScript lint errors") to sibling
packages declaring a nested function which references no binding local to its
immediate enclosing function, tripping `stdlib/no-unnecessary-nested-functions`.

Each declaration is dedented one scope level, as in the source commit. The JSDoc
block travels with the declaration and each body is unchanged apart from one tab
of indentation.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01SzTKV4KFPDZcVJF3NqTL9E
Reverts the dedent in `_tools/github/rank-followers` and
`_tools/github/rank-users`, leaving both files as on `develop`.

`test/test.cli.js` in each package fails identically on `develop` (2 and 4
assertions respectively): `bin/cli` keeps `format` and `delimiter` in locals
used only by the `csv`/`json` writers and never assigns them onto `opts`, while
`test/fixtures/cli_format.js.txt` and `cli_delimiter.js.txt` assert on
`opts.format` and `opts.delimiter`, which are therefore always `undefined`.
`cli_format.js.txt` additionally passes `--format=json` where the test expects
`csv`.

Touching either package makes those tests "affected", so the propagation
blocks on a defect it does not introduce and cannot fix without widening into
an unrelated test correction. Both sites remain valid and are logged for a
follow-up run once the CLI tests are corrected.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01SzTKV4KFPDZcVJF3NqTL9E

Copy link
Copy Markdown
Member Author

Run affected tests failed on 67f9bd78b (job) in _tools/github/rank-followers/test/test.cli.js. The failure is not this PR's, and b419426e5 drops the two sites it blocked rather than widening this PR into an unrelated test correction. Ten propagation sites remain.

The failure is pre-existing on develop

Reproduced locally against this branch and then against origin/develop's unmodified copy of each package, with identical results:

package test/test.cli.js on this branch on develop
_tools/github/rank-followers 25 tests, 2 failing 25 tests, 2 failing
_tools/github/rank-users 23 tests, 4 failing 23 tests, 4 failing

CI stopped at rank-followers, so the four rank-users failures had not yet surfaced.

Root cause

bin/cli never assigns format or delimiter onto opts. Both stay in locals consumed only by the csv/json writers:

fmt = flags.format || 'csv';
delimiter = flags.delimiter || ',';

opts only ever receives useragent, username, method and token. But test/fixtures/cli_format.js.txt and cli_delimiter.js.txt mock ./../lib and report opts.format / opts.delimiter back to the test, so those values are structurally always undefined — hence expected: 'csv', actual: undefined. cli_format.js.txt also pushes --format=json while the test asserts 'csv', so the fixture and its assertion disagree even on intent.

The assertions therefore test something the CLI is not designed to do. This is a test/fixture defect, not a library one.

Why the two sites were dropped rather than fixed here

The patches to those two files are pure dedents of an internal done callback, and the failing tests never execute them — the fixtures replace ./../lib wholesale via proxyquire. But Run affected tests derives its scope from the directories this PR touches, so touching either package makes the broken tests run. Correcting them means rewriting two fixtures and their assertions to exercise the CLI's actual output formatting, which is unrelated to the nested-function pattern this PR propagates.

Both sites remain valid and are logged for a follow-up run once the CLI tests are corrected. I found no open PR addressing them.

Proposed patch, for whoever picks this up

Assert on what the CLI actually emits rather than on the mocked opts. For the --delimiter case, keep the fixture's --format=json and have the mock return a { results, data } shape, then assert that stdout contains the delimiter in the CSV header; for the --format case, drop the opts.format assertion and instead assert that --format=json yields parseable JSON on stdout while the default yields a username,value CSV header. Alternatively, if format and delimiter are meant to reach the library, the fix belongs in bin/cli and the assertions can stay.

I have not spent a CI re-run: the failures are deterministic assertion mismatches, and running the same tests against an unmodified develop tree is stronger evidence than a re-run would be.


Generated by Claude Code

Reverts the dedent in `_tools/github/user-details`, leaving the file as on
`develop`.

`.github/workflows/scripts/run_affected_tests/run` expands the changed-package
set with every package that `require()`s one of them, and
`_tools/github/rank-followers` requires `@stdlib/_tools/github/user-details`.
Touching `user-details` therefore runs `rank-followers`'s test suite, which
fails on `develop`: 2 assertions in `test/test.cli.js` and 22 in
`test/test.pipeline.js`.

Dropping this site removes `rank-followers` from the affected set. Reproducing
the workflow's scoping locally now yields 13 packages and 77 test files, all
passing.

The site remains valid and is logged for a follow-up run once
`rank-followers`'s tests are corrected.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01SzTKV4KFPDZcVJF3NqTL9E

Copy link
Copy Markdown
Member Author

Correcting my previous comment: dropping rank-followers and rank-users was not sufficient, and I had the propagation mechanism wrong.

.github/workflows/scripts/run_affected_tests/run does not scope to the directories a PR touches. It expands that set with every package that require()s one of them:

dependents=$(find lib/node_modules/@stdlib -type f -name '*.js' \
  -exec grep -ol -E "require\( [']${escaped_package}['] \)" {} +)

_tools/github/rank-followers requires @stdlib/_tools/github/user-details, so touching user-details pulled rank-followers's suite back in even though this PR no longer modified it. That is why Run affected tests was still red on b419426e5, and why _tools/github/org-repos — a package this PR never touched — appears in the job log.

10ebcf947 drops the _tools/github/user-details site as well. rank-followers is now out of the affected set.

The pre-existing breakage is larger than I first reported

Reproducing the workflow's scoping locally and running all 83 resulting test files found rank-followers broken in two files, not one:

file failing assertions
_tools/github/rank-followers/test/test.cli.js 2
_tools/github/rank-followers/test/test.pipeline.js 22

Both are develop's: at b419426e5 this PR's copy of rank-followers was byte-identical to develop, so nothing here can account for them. The test.cli.js root cause is as described above (bin/cli never assigns format/delimiter onto opts); I have not diagnosed test.pipeline.js, since it is outside this PR either way. _tools/github/rank-users/test/test.cli.js carries a further 4 failures on develop and is no longer in scope.

Current state

Nine propagation sites remain: _tools/github/get (done in lib/factory.js), repl (clbk in lib/commands/load.js), and the seven repl/presentation onDrain handlers.

Verified locally before pushing, by reproducing the workflow's own scoping: 13 affected packages, 77 test files, all passing. The target lint rule reports zero violations across the nine files, and all seven dedented onDrain handlers were exercised at runtime against a stubbed presentation to confirm they still fire on drain.

Three sites are dropped and logged for a follow-up once rank-followers's tests are fixed: rank-followers and rank-users (done in lib/factory.js) and user-details (done in lib/factory.js).


Generated by Claude Code

Copy link
Copy Markdown
Member Author

Run affected tests is green on 10ebcf947, along with Lint Changed Files, Check licenses, Lint Copyright Years, Lint PR title and both Socket Security checks — 10 of 11.

The one remaining failure, Calculate test coverage for PR packages, is a defect in the coverage workflow script rather than anything in this diff. I am standing down on it rather than dropping seven validated fixes to work around it.

Mechanism

.github/workflows/scripts/run_tests_coverage/run filters _tools out of the changed set (line 129), so for this PR its package list is repl and repl/presentation. It then runs, per package (line 151):

make test-javascript-cov TESTS_FILTER=".*/${pkg}/test/.*" C8_FLAGS="-n 'lib/node_modules/@stdlib/${pkg}/**'"

repl/presentation has no test/ directory — no test files anywhere beneath it. So TESTS_FILTER matches nothing, and in test-c8 (tools/make/lib/test-cov/c8.mk:89):

$(QUIET) $(FIND_TESTS_CMD) | grep '^[\/]\|^[a-zA-Z]:[/\]' | while read -r test; do ...

the grep exits 1 on no matches. tools/make/Makefile:48 sets .SHELLFLAGS := -eu -o pipefail -c, so pipefail propagates that 1 and -e aborts the recipe — hence test-c8 Error 1 with no test output, 1.4 s after the repl suites all passed.

This is visible in the job log: every test that ran passed (repl integration suites 14 + 18 + 54 + 29 + 42 + 28, repl/test/test.cli.js 9, repl/test/test.js 2), then a second make invocation failed immediately without running anything.

Scope of the defect

Any PR touching a non-_tools package that has no test/ directory hits this. Scanning the tree, that is 3 of 3330 such packages: repl/presentation, plot/unicode/stemleaf and plot/base/ctor. Narrow, but repl/presentation is one of them.

Proposed fix, for a separate PR

Skip packages with no tests, before line 151:

if ! find "lib/node_modules/@stdlib/${pkg}/test" -name 'test*.js' -print -quit 2>/dev/null | grep -q .; then
    echo "No tests found for ${pkg}; skipping coverage." >&2
    continue
fi

Hardening test-c8 to tolerate an empty filter would also work, but it would mask genuinely mis-specified filters, so the guard in the caller seems preferable.

Why not worked around here

The only in-PR workarounds are to drop the seven repl/presentation sites — all independently validated, behaviour-preserving, and fixing real latent lint failures — or to edit CI config or add a test suite to repl/presentation. Both are outside this propagation. The coverage check computes a coverage-delta comment rather than gating correctness, and every check that does gate correctness is green.

No CI re-run spent: the failure is deterministic and structural, so a re-run would reproduce it exactly.

Final state

Nine propagation sites: _tools/github/get (done in lib/factory.js), repl (clbk in lib/commands/load.js), and the seven repl/presentation onDrain handlers. Three sites dropped for pre-existing test failures in _tools/github/{rank-followers,rank-users,user-details}, logged for a follow-up.


Generated by Claude Code

@kgryte
kgryte marked this pull request as ready for review September 26, 2026 20:29
@kgryte
kgryte requested a review from a team September 26, 2026 20:29
@stdlib-bot stdlib-bot added the Needs Review A pull request which needs code review. label Sep 26, 2026
@kgryte

kgryte commented Sep 26, 2026

Copy link
Copy Markdown
Member

As the CI failure is not due to changes introduced in this PR, I'll go ahead and merge.

@kgryte kgryte changed the title chore: propagate nested-function lint fix to _tools/github and repl chore: clean-up Sep 26, 2026
@kgryte
kgryte merged commit a8b875b into develop Sep 26, 2026
28 of 29 checks passed
@stdlib-bot stdlib-bot removed the Needs Review A pull request which needs code review. label Sep 26, 2026
@kgryte
kgryte deleted the philipp/fix-propagation-2026-09-26 branch September 26, 2026 20:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants