Skip to content

arb tool: orphans command, batch delete, clearer validate counts - #2442

Merged
ethicnology merged 3 commits into
developfrom
arb-tool-orphans
Aug 18, 2026
Merged

arb tool: orphans command, batch delete, clearer validate counts#2442
ethicnology merged 3 commits into
developfrom
arb-tool-orphans

Conversation

@basantagoswami

Copy link
Copy Markdown
Contributor

noticed some issues with the arb tool that this pr fixes

  • orphans: report locale keys absent from the template (unreachable, gen-l10n only reads template keys); supports --locale and --list
  • delete: accept multiple keys, all-or-nothing (aborts before writing if any key is unknown)
  • validate: print message keys and total entries separately so counts line up with missing/orphans/dead

@claude

claude Bot commented Jul 13, 2026

Copy link
Copy Markdown

Claude finished @basantagoswami's task in 2m 35s —— View job


Review: arb tool — orphans command, batch delete, validate counts

  • Gather context (diff vs origin/develop)
  • Review tools/arb.dart changes (orphans, batch delete, validate counts)
  • Post findings (as comment — inline review comments aren't available to me in this run, see note at bottom)

Overall this is a solid change: the orphans command correctly mirrors missing's structure (locale validation, template-locale rejection, --list), the batch delete keeps the pre-flight invariant check before any write, and the validate counts now genuinely line up with what missing/orphans/dead report (_realKeys excludes @metadata and @@locale on both sides). One real regression found, plus a minor completeness gap.

🐛 Regression: delete can no longer remove a leftover orphan @key

tools/arb.dart:484-486 — the new all-or-nothing pre-check only tests the base key:

final notFound = keys
    .where((k) => !files.any((f) => _readMap(f).containsKey(k)))
    .toList();

But _removeKeys (and the old code) also removes the @key metadata block. Before this PR, delete foo when only @foo existed (metadata with no base key) succeeded — _removeKeys removed the @foo block and removedFrom was non-zero. Now the pre-check sees no file containing foo and aborts with "Key(s) not found in any locale".

This matters because it's the documented escape hatch: add's guard at tools/arb.dart:304-314 explicitly rejects a leftover orphan @key and tells the user to run delete first — that recovery path is now broken (set doesn't help either: it requires the key in the template and never touches @key).

Failure scenario: template has a stray @foo with no fooadd foo ... errors telling you to delete first → delete foo errors "not found in any locale" → the file can only be fixed by hand-editing, which is exactly what this tool exists to avoid.

Suggested fix — accept a key when either the base key or its metadata exists:

final notFound = keys
    .where(
      (k) => !files.any(
        (f) =>
            _readMap(f).containsKey(k) || _readMap(f).containsKey('@$k'),
      ),
    )
    .toList();

Fix this →

🔍 Minor: orphans is blind to orphan @key metadata entries

tools/arb.dart:205-214 — both sides of the comparison use _realKeys, and the template locale is excluded from the scan. So the one kind of orphan the codebase already worries about — a @key metadata block with no base key (the exact case add's guard rejects, and the case in the regression above) — is invisible to the new command, whether it sits in the template or in a locale file. Similarly, a stray @key in a non-template locale (metadata is supposed to be template-only) goes unreported.

If limiting orphans to message keys is intentional (it matches how missing/dead count), that's defensible — but then the report can't find the thing its own suggested remediation (delete KEY...) is the escape hatch for. Worth either extending the scan or noting the limitation in the help text.

✅ Checked and fine

  • Batch delete atomicity: invariant pre-flight on every file before any write is preserved; the not-found check runs before the first _removeKeys, so a typo aborts with nothing written. toSet().toList() dedupes while preserving order (Dart's default LinkedHashSet).
  • orphans --locale en rejection and the no-positional-args guard mirror missing correctly; --locale/--list are registered in _commandOptions so the fail-loud option handling still works.
  • Validate counts: _realKeys(map).length matches the denominator missing uses (templateKeys.length) and what orphans/dead count, which was the point of the change.
  • README/help text match the new behavior.

Note: I couldn't post these as inline review comments (no inline-comment tool is available in this run, and gh api calls require approval), and the Dart SDK isn't available here, so this is careful static analysis rather than an executed repro. The regression is confirmed by comparing the old code path (_removeKeys(file, [key, '@$key']) with the after-the-fact removedFrom == 0 check) against the new pre-check.

@ethicnology
ethicnology merged commit d064899 into develop Aug 18, 2026
2 checks passed
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.

2 participants