0.7.0: nested-call wrap, record headers, parameter alignment, and layout decisions that stop reading layout - #50
0.7.0: nested-call wrap, record headers, parameter alignment, and layout decisions that stop reading layout#50barrycaceres wants to merge 49 commits into
Conversation
…dy max_width, multi-catch wrap
Three of the 0.6.1 scope items land here (E, A, D). The remaining
items (B+C+F+H idempotency spike; G Javadoc prose reflow) follow
in subsequent commits after their spike work is complete.
E — Class-body trailing side comment attaches inline:
`_emit_class_body_members` and `_emit_interface_body_members`
now call `_attach_trailing_side_comments` after each member
emission, matching the method-body iterators
(`_emit_indented_member_list`, `_emit_block`). A `//` comment on
the same source row as a field's `;` or method's `}` now stays
inline; pre-0.6.1 it moved to its own class-body-level line.
Locked by `comment_preservation/10`.
A — Single-arg lambda body excluded from P1 fit check:
`_emit_argument_list` single-arg cascade adds a `_is_block_body
_lambda` fast path that runs P1 with a fit check restricted to
the CALL LINE and CLOSING LINE. The lambda body's own line
widths are the body's responsibility. Pre-0.6.1 an over-80 line
inside a lambda body caused the entire outer arg-list to reject
P1 and cascade to P4, breaking `.method(() -> {` to `.method(\n
() -> {` and pushing the body +4 cols deeper — often creating
NEW overflows. Consumer trial on sz-sdk-java-grpc showed 22
sites (all `this.performTest(() -> { ... })` calls) regressing
this way; all 22 recover the baseline idiomatic shape.
Locked by `arg_list_wrap/15`.
D — Multi-catch union type wrap engine:
`_emit_catch_clause` gains a two-tier cascade for `catch (A | B
| C e)` when there are 2+ alternative types. P1 emits the whole
header inline if it fits under 80 chars (including the `{` that
follows). P2 breaks each subsequent `| Type` to its own
continuation line at the column right after `catch (`, with the
parameter name + `) ` on the last type's line. Fires C1
`multi-catch` overflow advisory if a single type name is itself
too wide at the paren-aligned column. Consumer trial site:
`sz-sdk-java-grpc/WrapperMain.java:66` — a 125-char four-type
catch that previously emitted silently. Locked by
`multi_catch_wrap/01`,`02`.
698/698 pytest passing.
…gate
Bug-fix release. Five formatter defects surfaced by trial-formatting
0.6.0 across four consumer source bases, plus a dependency bump the
drift guard makes impossible for Dependabot to land alone, and two
CI/tooling corrections.
Nested-call wrap (three rules). A call embedded in another expression
— a positional argument of another call, or the receiver of a chain —
no longer uses the P2 two-line comma-packed shape:
Rule 1: a sole method-invocation argument that cannot stay on one
line breaks so it lands at the enclosing call's line start + 4.
Rule 2: an embedded call's own arg list skips P2 (P1 -> P3 -> P4),
regardless of the enclosing call's arity.
Rule 3: chain segments following an embedded call go one per line,
anchored at the chain's own start column + 4.
`object_creation_expression` counts as a call for these rules.
Two layouts are now deliberately unreachable: the enclosing call left
inline with the inner arg list paren-aligned, and the first chain
segment hung off the inner call's closing paren with the rest
dot-aligned. Both required ranking two candidate continuation columns
against each other, which is not stable across passes — the mechanism
behind the builder(...).records(-1).build() oscillation. Selection is
now a single monotone "did the nested call stay on one line" test.
Source preservation declines for the two shapes these rules own, since
their layout is formatter-determined and echoing source rows
re-anchored them to the current emit column. This also stops a
preserved arg list from suppressing the chain cascade's Q-CHAIN-4
backoff.
switch brace placement. `switch (value)` now keeps its opening brace
on the same line per the standards' "Switch Statements and
Expressions" section. The emitter wrote an unconditional newline, so
every switch got an Allman brace — 98 sites across the four trial
trees, with the correct form never produced. Checkstyle does not gate
brace placement on LITERAL_SWITCH, so it went unnoticed. The
multi-line-condition exception still applies.
Also fixed: class/interface body trailing side comments now attach
inline; the single-argument cascade no longer counts block-bodied
lambda body widths in its P1 fit check (22 performTest sites in
sz-sdk-java-grpc had been rewritten); multi-catch union types now wrap
instead of emitting a silent 125-char line.
tree-sitter 0.25.2 -> 0.26.0. Binding only; tree-sitter-java stays at
0.23.5. Output byte-identical across all four trial trees with
matching advisory logs. requirements.txt now documents that
GRAMMAR_VERSION must be bumped in the same commit, since Dependabot
can only edit the pin and its PRs therefore always arrive red.
Build/CI: dropped the redundant /tooling/scripts/tests pip ecosystem
from dependabot.yml (both entries covered the same dependency set via
`-r ../requirements.txt`, so every bump arrived as two identical PRs);
added a corpus-gate job so the fuzz AST-round-trip, idempotency and
perf gates actually run — they resolve their corpus to <consumer>/src
and had been skipping on every CI run.
Verification: 704/704 pytest, six new nested_call_wrap fixtures.
Trial-formatted senzing-commons-java, sz-sdk-java, sz-sdk-java-grpc
and data-mart-replicator (504 files): zero AST changes, so no
formatting decision alters program meaning. Corpus idempotency 25 ->
14 non-idempotent with no new regressions. Lines over 80 went 1598 ->
1592; the three files that gain one each are unsplittable string
literals pushed over by rule 1's extra indent level.
…line
Two more defects from the four-project trial, both pre-existing in
0.6.0 rather than introduced by the nested-call wrap.
Pack-all-or-nothing for method chains. Removed the 0.6.0 "P1F"
factory-chain tier, which packed receiver + factory + the FIRST
CHAIN SEGMENT onto line 1 when the receiver was a PascalCase
identifier and the chain had 3+ segments. The all-on-one-line shape
is only available when the WHOLE chain fits; once it does not, the
break belongs at the first chain continuation dot, not wherever 80
characters ran out. The tier also rendered the same idiom two ways
depending on whether segment 1 happened to fit — Files.walk(a).sorted()
got the packed shape while Files.walk(b) kept the correct one.
Chains now fall through to P2F. Removes emit_p1f_factory, its
Q-CHAIN-4 signal, and the now-dead _chain_receiver_is_factory.
The same-method greedy tier (sb.append(a).append(b)) is untouched;
that horizontal density is deliberate. Shape-C sites across the four
trial trees drop 73 -> 24, and all 24 remaining are same-method
chains.
Arguments that wrap get their own line. Neither P1 nor P2 rejected a
shape where an argument was packed onto the call line and then
wrapped internally: every emitted line stayed under the cap, so the
width check passed and a partial break committed — the layout the
standards' "Anti-pattern" section forbids. Both tiers now reject an
ordinary argument that had to wrap, leaving it room to render whole
one line down.
Arguments that inherently own multiple rows (block-bodied lambdas,
text blocks) are exempt, so performTest(() -> { ... }) keeps P1. The
exemption tests only structural properties of the node and never the
source layout — consulting the source makes the answer depend on
whether an earlier pass already wrapped the argument, which
oscillated arguments(Rectangle.class, Set.of(...), ...) between two
shapes on alternate passes.
Fixture 18 renamed from 18_p1f_factory_deep_dot to
18_factory_chain_breaks_at_first_dot to describe what it now locks.
Fixtures 05/06/07 in arg_list_wrap and 14/16 in method_chain_wrap
take the new shapes; all were reviewed and confirmed as
improvements.
Verification: 704/704 pytest. Across the 504-file trial: zero AST
changes, lines over 80 went 1598 -> 1592, corpus idempotency 25 ->
12 non-idempotent with no new regressions, shape C 73 -> 24, and
switch-Allman 98 -> 0.
…s' rule Documents the behavior committed in 763f682. An argument too wide for the space left on the call line must not be packed onto it and then wrapped internally — that satisfies the 80-char limit while still producing the partial-break anti-pattern, since the argument's continuation column ends up set by where the line ran out rather than by structure. Covers the scope explicitly: it applies to any argument complex enough to wrap (nested call, lambda, object creation, compound expression such as a long concatenation), does not affect simple arguments — which cannot wrap and so keep priority 2 two-line packing — and exempts block-bodied lambdas and text blocks, for which spanning lines is inherent rather than a wrap.
…gaps Applies the unambiguous findings from the Opus code review of e3d52e9..be1c04e. The one MUST-FIX and two judgment calls are held back for a decision. Rule 1 now covers object_creation_expression. CHANGELOG and the _is_nested_or_chained_call docstring both already stated that `new Foo(...)` counts as a call for these rules, but rule 1 and the source-preserve decline tested only method_invocation — so a sole `new Foo(...)` argument got rule 2 without rule 1 and landed in the deep-column shape rule 1 exists to prevent. Anonymous classes are carved out. `new Runnable() { ... }` parses as object_creation_expression, so the above change initially broke the idiomatic spec-C8 shape by pushing the whole body a level deeper. New _is_anonymous_class helper; an anonymous class body owns its rows the way a block-bodied lambda does, so it is excluded from rule 1, from the source-preserve decline, and from _arg_owns_its_rows. Caught by test_anonymous_class_as_call_argument. Test gaps closed: - TestGrammarVersionPins compared two files and never the environment, despite a docstring claiming otherwise — the review passed 704 tests with tree-sitter 0.25.2 installed against a 0.26.0 pin. New test_installed_versions_match_pins checks importlib.metadata. - New switch_brace fixtures (statement, expression, multi-line condition). The 98-site brace change had only incidental coverage and the Allman branch had none. The multi-line case uses a binary condition because a wrapping call condition exposes pre-existing two-pass drift that also affects if/while under 0.6.0. - 17 table-driven tests for _is_nested_or_chained_call and _is_anonymous_class, pinning the True cases and — equally important — the parent shapes the rules deliberately do not reach (parenthesized, cast, ternary, lambda, binary, field access, array access, statement-level). Cleanups: corrected the docstring claiming a constructor cannot be a chain receiver (it can — `new Foo(a).bar()` nests it as the object field); deleted a superseded rule-3 comment block whose description did not match the code; removed a duplicated named-argument list; dropped a dead `if segments` guard; explained why p3_col takes a max; and collapsed a duplicated advisory-and-return into a fall-through. Docs: fixture-golden count corrected from two to nine plus the renamed directory; before/after example made the same call; rule 3's narrower scope stated in the definition rather than contradicted 90 lines later; the NOT-PRODUCED caption no longer blames paren-alignment the document endorses elsewhere; three cspell words reworded rather than whitelisted. Verification: 725/725 on the pinned tree-sitter 0.26.0 (the suite now refuses to run green on a stale binding). Release gates unchanged against 0.6.0 across 504 files — over-80 1598 -> 1592, non-idempotent 25 -> 12 with no new regressions, shape C 73 -> 24, switch-Allman 98 -> 0, zero AST changes.
identifier-receiver dot-align
Closes the review's MUST-FIX by rolling in the deferred
source-preservation work, and applies the one judgment-call
SHOULD-FIX.
MUST-FIX — preservation bypassed the break rule. "If an argument
breaks, the argument list breaks" lives in the wrap engine's P1/P2
commit checks, but source preservation is consulted first and
short-circuits the cascade. An arg list authored in the
packed-then-wrapped shape was echoed straight back, so the formatter
emitted the exact layout the standards document publishes under "NOT
PRODUCED", and two semantically identical inputs formatted
differently depending only on how they were typed. The escape hatch
was a cast_expression argument, which _is_nested_or_chained_call
declines; parenthesized and ternary have the same hole. Fixed at the
class level rather than by enumerating parent shapes: preservation
now declines when any argument spans rows in source and is not one
that owns its rows. _arg_owns_its_rows is promoted to module level
for the purpose, keeping block-bodied lambdas, text blocks and
anonymous classes on the preservation path so
`execute(new Runnable() { ... })` stays idiomatic.
Preserved continuations are re-anchored. The path replayed author
columns literally, so re-indenting the enclosing statement left the
continuation aligned with nothing (67 sites in sz-sdk-java). Rows now
shift by the construct's own displacement, and the previously
separate "respect a deeper author indent" and "shift to target"
branches collapse into one delta: max(re-anchor, to-target). The
to-target term is a floor — an unfloored re-anchor drove
continuations to column 0 when the displacement was large and
negative, which the corpus idempotency gate caught. Idempotency holds
by construction: on a later pass the source column is the emit
column, so the re-anchor term is zero.
SHOULD-FIX 8 — the chain P2 suppression now also requires
`head is None`. It exists for chains rule 1 broke out, whose receiver
is itself a wrapping call; when the receiver is a bare identifier the
dot-aligned form is readable and compact, and suppressing it turned a
common test idiom into four lines for no benefit. Now matches the
`head is None` condition p3_col already used, so the tail anchor and
the tier gate key on the same shape.
Three fixtures lock the new behavior: a pre-wrapped cast argument
being re-wrapped, a continuation re-anchoring on dedent, and an
identifier-receiver chain keeping its dot-align.
Verification: 728/728 on the pinned tree-sitter 0.26.0. Across the
504-file trial versus 0.6.0 — over-80 1598 -> 1590, non-idempotent
25 -> 11 with no new regressions, shape C 73 -> 24, switch-Allman
98 -> 0, zero AST changes.
…ot fit
Deep-orphan fix. An argument list whose opener sits mid-line at a deep
column could emit an argument at a shallower anchor than the `(` it
belongs to — drift up to 49 columns — because the argument's own
cascade ran out of tiers and committed its block-relative C1 fallback,
whose anchor has nothing to do with that list.
The old code chose paren-alignment because the EARLY arguments fit,
then discovered a later one could not and orphaned it. That is why the
same construct rendered correctly at one argument and wrong at the
next:
new SzInterestingEntity(100L,
1,
Arrays.asList(
"FLAG"), <- correct
Arrays.asList(
createSampleRecord("DS", "R"))); <- orphaned at col 12
Rule, per the 0.6.2 scope decision: if ANY argument cannot render at
the paren-align column without escaping to a shallower anchor, then
ALL arguments behave as if the first had not fit, and the list
escalates to P4 (break before the first argument, one per line at
line-start + 4):
new SzInterestingEntity(
100L,
1,
Arrays.asList("FLAG"),
Arrays.asList(createSampleRecord("DS", "R")));
Testing every argument is the point — the decision is made once for the
whole list instead of being discovered partway through.
Measured across the 504-file trial, orphans fall 37 -> 12 (0.6.1 alone
had reached 21). The shapes this replaces were badly mixed, e.g.
assertEquals with a chain argument stranded at col 12 under a col-21
paren-align now renders every argument in one column with the chain
properly nested.
TRADE-OFF, flagged deliberately: lines over 80 move 1590 -> 1599. The
additions are concentrated in five files and are almost entirely one
repeated string literal that now sits four columns deeper because a
lambda body is correctly nested one level in — where it previously sat
at col 12 beneath a col-35 paren-align, which was the defect. Correct
nesting costs those columns; the literal needs a manual split and the
C1 advisory fires on it. This is checkstyle-visible and worth
confirming before release.
Remaining 12 orphans have a different root cause: 7 are chain segments
(.addRecord(, .getEntityByRecordId(, .endsWith(), where the CHAIN
should back off to a shallower tier via Q-CHAIN-4 rather than the
argument list escalating. Tracked separately.
Verification: 729/729 on the pinned tree-sitter 0.26.0, zero AST
changes across 504 files, non-idempotent 25 -> 11, no new
non-idempotent files, no fixture churn beyond the new lock.
Second half of the deep-orphan work. A chain RHS emitted inline can
push its tail to a column where the tail's own arguments do not fit;
the argument list then reaches its block-relative last-resort tier,
whose anchor bears no relation to the `(` it belongs to:
String nativeResult = engine.getNativeApi()
.getEntityByRecordId(
dataSourceCode, recordID);
Step 3 of the declarator cascade could not detect this: every line
above is under 80, so its overflow test passes and it commits. The
signal is not width.
New `Emitter._anchor_escaped`, set at the one place the escape happens
— the argument-list paren-defer candidate's block-relative branch —
and read by the declarator to treat the inline shape as failed and
back off. Added to __slots__ and to snapshot()/restore() so
speculative emits unwind it, matching `_arg_list_p4_fired`.
Geometry was tried first and rejected: the legitimate chain-P3 ladder
(segments at block+4) also places rows left of the value column, so a
column test cannot tell a clean ladder from a real orphan. It
false-positived on method_chain_wrap/02, 03, 06, 07 and 11. An
explicit flag at the escape site has no such ambiguity, and all five
of those fixtures pass unchanged.
A narrower first attempt -- preferring break-at-`=` for any wrapping
multi-segment chain -- was also rejected: it cost a line on every
wrapped chain that orphaned nothing, which is most of them. The
backoff is a remedy, not a preference.
SCOPE NOTE: this changes zero files across the 504-file trial today,
because the five real chain sites are blocked upstream by a different
mechanism -- their orphan is ALREADY IN THE SOURCE (e.g.
SzCoreEngineReadTest.java:110-111) and source preservation echoes it
back verbatim. Those are self-perpetuating orphans written by an
earlier release. Declining preservation for an orphaned layout is the
missing piece; this commit is what produces the correct shape once
that lands.
Verification: 730/730 on the pinned tree-sitter 0.26.0. Trial gates
unchanged -- orphans 37 -> 12, non-idempotent 25 -> 11, zero AST
changes, no new non-idempotent files.
…or wrap Three changes that only work as a set. Retiring the preservation fallback alone was NOT a clean win; the other two make it one. 1. Source preservation's width-based fallback is retired. Preservation keeps its two correctness triggers (interleaved comments, CSOFF) and drops the third: "the source spans rows and its first line fits, so echo the author's layout". That fallback is a propagation channel for the formatter's own past output -- the deep orphan at SzCoreEngineReadTest.java:110-111 survived every pass because it IS in the source and the gate re-emitted it. It also made layout history-dependent: two semantically identical files formatted differently according to how they were typed. 2. New P4-packed tier: break after `(` and put ALL arguments on one continuation line at line_start + 4. This is the zero-args-on-the-call-line member of the greedy family, whose rule is two lines maximum -- zero or more arguments on the call line, all remaining on ONE continuation line, else fall back to one per line paren-aligned. Sits inside the same rule-2 guard as P2, because it is a greedy tier and rule 2 withdraws greedy shapes from embedded calls; outside the guard it re-introduced the packed-inside-an- enclosing-construct shape rule 2 removed. 3. Enhanced-for headers now wrap: primary break BEFORE the `:`, colon leading the continuation line, Allman brace when the header breaks -- the same Multi-line Conditions exception that governs if/while/ switch. Previously written straight out with no cascade, so 25 sites simply overflowed. Why the set matters: (1) alone cost +660 lines, because the author's packed block+4 shape is not in the formatter's vocabulary and every such site expanded to one argument per line. (2) supplies that shape, turning +660 into -168. (1) also EXPOSED the gap (3) fixes -- preservation had been wrapping long enhanced-for headers by accident. Measured across the 504-file trial vs the prior tip (f0ba014): lines over 80 1599 -> 1578 enhanced-for over 80 25 -> 0 non-idempotent files 11 -> 8 deep orphans 12 -> 3 AST changes 0 -> 0 files changed 265, net -168 lines Three files gain one over-80 line each; all are literal-dense demo or flag files already carrying hundreds. 732/732 pytest on the pinned tree-sitter 0.26.0. One existing golden updated -- explicit_constructor_invocation/01 now takes the formatter's own P2 cascade rather than the echoed author layout, which is the point of change (1); its tail-reserve purpose is still exercised.
…ANGELOG Documentation and labelling for the three changes folded in from the preservation spike. No behavior change. Standards document: - New "Priority 3b: Next-line, all arguments on one line" between priorities 3 and 4, stating the greedy family's two-line-maximum rule explicitly (zero or more arguments on the call line, all remaining on ONE continuation line, else one per line paren-aligned) and noting it is skipped for embedded calls like priority 2. - New "Enhanced-`for` header wrapping" section: break before the `:` with the colon leading the continuation, Allman brace when the header breaks, cross-referencing the Multi-Line Conditions rule it follows. Includes the fits-on-one-line counter-example. CHANGELOG: the three separate "Source preservation" sections are consolidated into one covering all three narrowings (fallback retired, continuations re-anchored, yields to the nested-call rules). Verification figures updated to the folded-in totals: 732/732, over-80 1598 -> 1578, non-idempotent 25 -> 8, orphans 37 -> 3, 265 files reformatted for a net -168 lines, plus a note that retiring the fallback alone would have cost +660 and priority 3b is what turns that into a reduction. Version labels normalised: nine `0.6.2` markers in format_java.py became `0.6.1`, and `.claude/062_SCOPE.md` is renamed `061_REMAINING_SCOPE.md`, since all of this ships as 0.6.1. Gates: 732/732 on the pinned tree-sitter 0.26.0; CI corpus-gate simulation 218 passed; prettier, cspell and markdownlint clean across every added range.
…ndent Promotes the release from 0.6.1 to 0.7.0. It adds normative rules to the standards document and removes two behaviors that document described, so the minor bump is the honest version: 322 of the 504 trial-corpus files are reformatted. Record headers wrap. They had no cascade at all, so an over-long header simply stayed over-long. The spec's "Record Headers" priorities are now implemented as written: `implements` moves to its own line first, and the components only break if they still do not fit. Components run the shared parameter cascade with `force_wrap=True`, which also retires source preservation for them -- preservation was replaying an author's packed layout as an 88-column row. A component list that wraps itself and leaves `implements` trailing its closing paren is now rejected explicitly; width alone accepted it because every row was under the limit. Wrapped parameter lists align their names. The spec has always required names at the first 4-space tab stop past the longest type, and the cascade never generated it -- every aligned list in the corpus was author-written and source-preserved, leaving the corpus split roughly evenly between aligned and single-spaced lists. Single parameters are never padded (no column to form) and lists with varargs or receiver parameters are not padded (their prefix is not a bare type). Both carve-outs are documented. Indented javadoc lines are structural, not prose. This fixes the last non-converging construct in the corpus: paragraph runs split at non-prose lines, and reflow then erased the indent that did the splitting, so each pass regrouped and reflowed differently. Javadoc prose reflow also now balances rather than packing greedily when greedy strands three words or fewer, sharing one helper with `//` reflow; scoped to two-line paragraphs because the soft-target rebuild misbehaves at three or more. Round-2 review fixes. The argument-list escalation scanned one row too many -- `line_count` excludes the in-progress line, so for argument 0 the scan began on the call line, whose indent is always left of the continuation column, making every wrapping first argument report a false escape and skip priority 3. The wrapped enhanced-`for` path reserved nothing for its own closing paren and landed it in column 81, silently and idempotently. The variable declarator no longer clears an enclosing construct's `_anchor_escaped` on its way out. Dead code removed. Retiring the source-preservation width fallback left every remaining branch in `_arg_list_takes_source_preserve_path` returning the same False as its fallthrough -- around 155 lines that read as live policy, one of them walking the AST 2,113 times across 250 files to compute an unobservable answer. Those branches and the helpers that served only them are gone; output is byte-identical across all 504 files. The reasoning is preserved in a new `source-preservation-history` FAQ. Priority 3b is renumbered 2b and moved in the document to where it runs: after priority 2, before priority 3. Measured against 0.6.0 over 504 files: lines over 80 1618 -> 1581, files needing a second pass to settle 26 -> 6 with none failing to converge, deep orphans 37 -> 3, over-long enhanced-`for` headers 25 -> 0, one structural AST change (an else-less Tier 1 brace collapse), no new parse errors. 722 tests pass on the pinned tree-sitter 0.26.0.
…d code Round-3 code review plus edge-case probing found two ways the new parameter alignment could make output worse, and two places the formatter declined to act without saying so. Alignment no longer overflows the terminal candidate. Padding a short type out to a long type's column carries the short parameter's full name with it, so a compliant 50-column line became 82. Priority 3 has no tier below it, so that committed. The aligned and unaligned forms are now compared, and alignment is given up only when doing so actually buys width back. Comparing them matters: testing the aligned form against the cap alone consulted `last_lines_max_width`, which starts at the row already open when the parameters began — the signature row — and that stripped alignment from three files whose parameter rows were nowhere near the limit purely because the method name above them reached column 80. Double-indenting only when it gains room. Priority 3 breaks after the `(` to escape a paren column pushed right by a long return type and name. When the paren already sits at or left of the double-indent column, that break moves every parameter further right, so priority 2 is kept instead. The test is the column of the `(`, not the length of the method name, so return type, modifiers and type parameters all count. No corpus file changes; the shape needs a very short signature beside unusually long parameters. Two silent exits now report. A parameter list that cannot fit with every parameter on its own line at the deepest indent had no advisory at all, and a javadoc line preserved as structural had none either. Both are spec C1 emit-and-warn sites missing their warn half, and the failure mode was a developer hitting a checkstyle LineLength failure, running the formatter, and getting neither a change nor an explanation. Both respect checkstyle's own LineLength ignorePattern, without which `@see <a href=...>` javadoc alone produced 123 unactionable advisories; with it the release adds 17, each naming a line that genuinely fails the build. Dead code the previous commit left behind: `_arg_list_takes_source_ preserve_path` still carried a `column` parameter, a `col` local and a docstring contract describing the removed width triggers, none of which survived the retirement. Parameter and docstring removed, both callers updated. Also dropped the unused `re` import, the orphaned `_ESTIMATE_VERBATIM_NODE_TYPES` constant, and an unreachable branch in the javadoc paragraph scan that the structural-indent change subsumed. A record with no `implements` clause no longer re-emits its components to no effect. Corrections to this release's own documentation: the standards document claimed receiver parameters are emitted unpadded when they are in fact silently dropped (pre-existing, now recorded where a reader will find it); the cited spec section name was wrong; four CHANGELOG figures were wrong, including a subtraction and a 6-versus-7 mismatch; and a claim to have fixed "the last non-idempotent construct" conflated never settling with needing a second pass. Three constructs still need a second pass and all converge on it; what this release removes is output that never settles. Corpus unchanged by every fix in this commit except where stated: 1581 lines over 80, 6 files needing a second pass, all converging. 723 tests pass on the pinned tree-sitter 0.26.0.
The rule was enforced at priorities 1 and 2 but not priority 3, so a
paren-aligned argument that wrapped internally put its own continuation
at the column its SIBLINGS use, and the continuation read as another
argument:
multilineFormat(rr.getFormat()
+ " record not as expected:",
"RECORDS TEXT: ",
Breaking the whole list moves the arguments to their own column and
leaves the continuation clearly subordinate. Arguments that inherently
own rows -- block-bodied lambdas, text blocks, anonymous classes -- stay
exempt, as everywhere else this rule is applied. The round-2 off-by-one
fix to the escape scan is what made priority 3 reachable for wrapping
first arguments, which is why the gap became visible now.
Also fixes a latent off-by-one the change exposes: emit_p4_multi_arg
appends a comma or closing paren after every argument and reserved for
neither, so an argument measuring itself as exactly 80 wide committed and
the separator landed in column 81 -- invisible to the argument, which
fit, and to the loop, which had already committed.
That reserve drops the INHERITED tail reserve for every argument but the
last. It stands for characters the parent appends after the whole
construct (an enclosing statement's semicolon), which land on the final
line only; carrying it onto a middle argument's row makes the budget one
char too tight and splits an 80-column argument that is legal. This is
the fourth off-by-one of the same family found in this release.
Measured on the 504-file corpus against the previous commit: lines over
80 unchanged at 1581, total lines +660 (+0.30%), 121 files change shape,
files needing a second pass unchanged at 6 with all converging,
advisories 291 -> 290. Total files reformatted against 0.6.0 moves only
322 -> 324, because those 121 files were already in the reformat set.
Spec text for the rule at priority 3, a CHANGELOG entry for it and for the argument-separator reserve, and the seven fixture goldens the change moves. Five of the seven are improvements -- a binary, chain or nested call that was split at the paren-aligned column comes back whole; two cost a line moving from paren-aligned to block+4, which is the shape the rule exists to produce. Verification figures updated for the combined release: 324 of 504 trial files reformatted, net +1,491 lines against 0.6.0.
…reserve
Round 4 found two ways the new priority-3 escalation fires on rows the
argument list did not put there, and corrected its own earlier clearance
of the second.
Both priority-3 escape signals now share the same exemptions. A row left
of the continuation column is evidence of an escape only when this
argument list's cascade chose that anchor; a text block's content starts
where the author wrote it, often column 0, and an argument replaying
source rows carries its own columns. `p3_arg_escaped` tested every row
regardless, pushing 8 corpus lines over 80.
The array-creation source-preserve path is retired. It replayed a
multi-row `new Type[] { ... }` verbatim with no re-anchoring, so when the
escalation moved the argument's first row to block+4 the preserved
continuation stayed where the author left it, leaving the two halves of
one argument 24 columns apart as a stable fixed point.
Suppressing the escalation for preserved arguments was implemented first
and then rejected: it made the choice of shape depend on whether the
array happened to be written across rows, which is exactly the
history-dependence this release exists to remove, and it left
AbstractSchedulingServiceTest.java oscillating between two shapes.
Retiring the path makes array layout a function of the AST. Verified
layout-independent: the same call written flat and pre-split now produces
identical output.
A new `Emitter._raw_rows_emitted` flag, set inside `write_raw_lines` and
read save-reset-check-restore like `_arg_list_p4_fired`, lets a wrap
decision distinguish "this construct wrapped" from "this construct's rows
came out of the source" -- indistinguishable in `line_count` but opposite
in meaning. It still guards the four preserve channels that remain
reachable from argument emission: switch rules, formal parameters, and
the argument-list comment and CSOFF cases. Correct for comments and CSOFF,
which are content rather than layout; a stopgap for the other two.
Measured on the 504-file corpus: lines over 80 1581 -> 1573, total lines
-54, advisories 290 -> 282, files needing a second pass back to 6 from
the 7 the rejected stopgap caused, 324 of 504 files reformatted against
0.6.0, one structural AST change (an else-less Tier 1 brace collapse),
zero parse errors. 723 tests pass on the pinned tree-sitter 0.26.0, with
no fixture goldens moved.
S1 — the argument-separator reserve had no test. Replacing its `(tail_reserve + 1) if is_last else 1` with the uniform `tail_reserve + 1` used at every other reserve site passed all 723 tests, so a maintainer normalising it to the surrounding idiom would have seen green while costing 26 corpus lines. Two fixtures added, and the first was checked to actually discriminate: `19_middle_argument_keeps_full_budget` fails under the uniform reserve, and `20_wrapped_argument_escalates_past_paren_align` locks the priority-3 escalation. A first attempt at the guard did NOT discriminate — its argument measured 77 columns, inside both budgets — which is why the discriminating width was found from a real corpus site rather than assumed. S2 — five fixture names contradicted their regenerated goldens, worst being `18_arg0_wraps_but_paren_aligned_still_fits`, whose golden now shows paren-alignment deliberately not used. Renamed to describe what they lock. S3 — the spec justified the priority-3 rule as the continuation landing at its siblings' column, which is true of a binary concatenation and false of a nested call, whose continuation sits at its own paren column and is never ambiguous, yet still breaks the list. The rationale now states the real reason, uniformity, and "every priority" is qualified to exclude priority 4, the terminal fallback where wrapping is permitted of necessity. S4 — the spec's headline example did not reproduce: as written it formats to priority 2, producing neither documented shape. Replaced with the shape and depth that actually reach priority 3, with a note that the depth is what gets it there. S5 — completed a truncated CHANGELOG heading. S6, N1 — the CHANGELOG implied the change usually saves lines while illustrating it with a shrinking example. Of 126 shape-changed files 94 grow, 15 shrink, 17 hold; and 124 of the 126, not all of them, were already in the reformat set. N2, N3 — reworded a comment that stated its worked example on two different bases, and corrected a comment block that still presented fixture 18's paren-aligned shape as the outcome the escape-scan fix produces. The argument-breaks rule now escalates that case regardless; what the fix still buys is that the escape signal means what it says. N4 — recorded that the pytest figure needs a consumer checkout: the fuzz corpus skip-marks without one, and the 210 skipped parametrisations are precisely the AST-equivalence and idempotency checks. N5 — deleted the dead `any_multiline_arg`, a source-dependent computation with no readers. Corpus output byte-identical to the measured build; 725 tests pass.
…e venv The spellcheck workflow runs on every PR to main and would have failed: ten words introduced by this release's commits were not in the dictionary (`illegit` in eleven places, plus `neighbours`, `idempotently`, `unfloored`, `dedented`, `ungated`). Fixed by rewording the prose and renaming `p1_illegit_wrap` -> `p1_invalid_wrap` and its siblings, per the repo's rule against whitelisting invented words. Only `venv` was added to the dictionary, a real tool name sitting beside the existing `virtualenv` entry. Verified behaviour-neutral: corpus output is byte-identical across all 504 trial files and the suite is unchanged. The consumer-trial checklist's idempotency gate demanded `0 modified` on the second formatting pass, which this release does not satisfy and no recent release has. It conflated two different outcomes. The gate is now convergence to a fixed point: a small tracked set of files changing on the second pass and then settling is tolerated and reported, while a file that never settles, or oscillates, is blocking. That is the distinction this release actually turns on -- 26 files needed a second pass under 0.6.0 and 6 do now, with none failing to converge. New FAQ `building/formatter-python-environment` covers the pinned tree-sitter environment and the three-way check between `requirements.txt`, `GRAMMAR_VERSION` and the installed distribution -- the trap being that a stale interpreter runs the suite green while the pinned grammar goes untested.
Super-linter summary
All files and directories linted successfully For more information, see the GitHub Actions workflow run Powered by Super-linter |
🤖 Claude Code ReviewCombined Code ReviewReview Part 1 of 2Code Review — Part 1 of 2Reviewed the tooling/docs changes for the 0.7.0 formatter release (CHANGELOG, standards docs, CI, and Code Quality
Testing
Documentation
Security
I'll fold in a review of Part 2 once it arrives to complete the full picture (particularly test coverage, which is currently the biggest open checklist item). Review Part 2 of 2Code Review — Part 2 of 2(Note: only Part 2's diff was provided to me; I cross-checked the claims below against the actual repository state at HEAD, which reflects the full merged change, to verify things the diff alone couldn't confirm.) Code Quality
Testing
Documentation
Security
Summary: No defects found in this half of the diff. The two areas that looked most likely to hide bugs (test deletion, and the snapshot/restore handling around the new orphan-detection flag) both check out correctly against the current codebase. Automated code review analyzing defects and coding standards |
🤖 Claude Code ReviewCombined Code ReviewReview Part 1 of 2I reviewed Part 1 of the diff (docs, CI/config changes, and the bulk of Code Quality ✅ mostly — the refactor is unusually well-documented (rationale comments throughout), DRY is respected ( Testing — no test files appear in this chunk of the diff; can't assess coverage from Part 1 alone (may be in Part 2). Documentation ✅ — CHANGELOG, standards doc, and two new FAQ docs are thorough and cross-referenced. Couldn't run prettier/cspell directly (sandboxed bash commands with certain grep patterns were blocked by approval requirements), so CommonMark/formatting compliance is based on visual inspection only, not tool-verified. Security ✅ — no hardcoded credentials, no Both findings above are low/medium severity — nothing blocking. Waiting on Part 2 of the diff to complete the full review. Review Part 2 of 2Code Review — Part 2 of 2Scope note: Only part 2 of the diff was provided in this context (no part 1), so this review covers the second half of the changes: Code Quality
Testing
Documentation
Security
SummaryNo defects found in this half of the diff. The changes are unusually well-documented for a formatter internals change — each removed/added tier includes a concrete before/after example of the bug it fixes or the shape it eliminates, and the two subtlest changes (anchor-escape restore, sole-arg chain gating) check out correctly against the actual committed source. Only suggestion: add a small direct unit test for Automated code review analyzing defects and coding standards |
The wrap engine treats every named child of an `argument_list` as an
argument, and tree-sitter exposes a comment as a named child, so a comment
between arguments was counted as an argument and given a separator. The
formatter emitted Java that does not parse:
in outer.call(inner(alphaValue, /* note */ betaValue), tag);
out outer.call(inner(alphaValue, /* note */, betaValue), tag);
Source preservation is already the answer to "the wrap engine has no
concept of an inter-argument comment" -- that justification is written in
the predicate. It was simply gated behind the multi-row test, so a
single-row list fell through to the engine. The comment check now runs
first, independent of row span.
Latent rather than live: no argument list in the 504-file trial corpus
contains a comment child, which is why four review rounds did not surface
it. Corpus output is byte-identical after the fix.
Found by the round-4 review of PR #50, which initially cleared this by
testing only a trailing `//` and a leading `/* */` on its own row -- both
of which already hit a preserve path.
`_emit_formal_parameters` filtered its parameter list to
`formal_parameter` and `spread_parameter`, and no emitter was registered
for `receiver_parameter`, so an explicit receiver was silently discarded:
in void method(T this, String name)
out void method(String name)
Dropping a bare `T this` is semantically inert -- the construct exists
only to give annotations somewhere to attach -- but dropping an annotated
one discards the annotation, which is not inert. Either way the formatter
was altering source it was asked to format, and silently, where its
convention for an unsupported construct is an explicit refusal.
Receiver parameters are now emitted verbatim, which preserves annotations
exactly. They stay out of the name-alignment machinery: it has nothing to
align here, and `_formal_param_name_col_offset` already declines any list
whose members are not all plain `formal_parameter` nodes, because one
measured prefix width would not describe them.
No occurrence in the 504-file trial corpus, so corpus output is unchanged.
`field_access` had no wrap tier. Its reserve was correct -- the receiver
emits under `tail_reserve + 1 + len(field)` -- but when the receiver
exhausted its own cascade and committed its terminal candidate, the field
was appended to a row that was already full:
int n = methodBeingCalled(
argumentOne,
argumentTwo).someFieldNameHere; <- 85 columns
A fitting shape existed and the formatter could not reach it. Breaking
before the `.` is already the documented rule -- the spec lists `.` among
the break-before operators -- so the field now moves to its own line at
single indentation when the inline form does not fit.
Restricted to a COMPUTED receiver: a call, array index, object creation,
cast or parenthesized expression. The same node type also spells qualified
names and constant references, where the dots belong to the name, and a
first attempt at this tier shredded them:
java Boolean
.util .FALSE.equals(...)
.Objects.requireNonNull(...)
Caught by fixtures `arg_list_wrap/09` and `nested_call_wrap/06`. The
narrowed tier also declines when breaking buys no width, since a receiver
that overflows on its own is not helped by moving the field off its last
row and the inline form costs one line fewer.
Corpus output byte-identical across all 504 trial files; the shape needs a
call whose arguments reach the limit before a field access is applied to
it. Correcting my own earlier description: this was never a missing
reserve, and the case did emit an advisory before the fix.
A value that commits at exactly 80 columns leaves the `;` in column 81,
and the formatter said nothing. The wrap engine's emit-and-warn exits run
while the semicolon is still unwritten, and `tail_reserve` does not carry
it, so the advisory measured 80 and declined to fire. The result is
idempotent, so it survives every reformat -- and it takes compliant source
and makes it non-compliant with no output at all:
in String s = Option.sourceDescriptor(
COMMAND_LINE, CONFIG, "--config");
out String s
= Option.sourceDescriptor(COMMAND_LINE, CONFIG, "--config");
The check now runs after the `;` is written, so it measures what actually
reaches disk. 23 declarations in the 504-file trial corpus report, at 81
to 84 columns; none matches checkstyle's LineLength ignorePattern, so
every one is a line the build will reject.
This REPORTS only. Threading the semicolon into `tail_reserve` so the
value's own cascade breaks earlier was implemented and reverted: it fixed
the target case but double-charged the semicolon against the declarator's
hardcoded `+ 1` allowance, and after correcting that it left
`MessageConsumerFactory.java` changing on every pass -- the one failure
mode 0.7.0 eliminated. Layout is deliberately unchanged here; corpus
output is byte-identical, over-80 stays 1573, and files needing a second
pass stay 6 with none failing to converge. The layout fix is deferred.
Three change behaviour and one adds an advisory; all four were found by
the round-4 review of this PR and all four reproduce identically at
0.6.0, so none is a regression from this release.
1. Inline argument comments emitted Java that does not parse.
2. A declaration whose semicolon lands in column 81 said nothing.
3. Field access could not break before its dot, stranding an 85-column
line the cascade had a fitting shape for.
4. Receiver parameters were silently dropped, discarding annotations.
Corpus layout is byte-identical to the release build: none of the three
behavioural fixes occurs in the 504-file trial corpus, which is why they
survived four review rounds, and the fourth reports without relaying out.
over-80 stays 1573, files needing a second pass stay 6 with none failing
to converge, and 23 declarations newly report at 81 to 84 columns.
Two more decisions were reading source layout the formatter then rewrites, so pass 1 answered from the author's layout and pass 2 answered from pass 1's own output. Neither changes the fixed point: all 504 corpus files converge to byte-identical output, with lines over 80 (1571) and advisories (301) unchanged. They reach it a pass sooner. **Basic-`for` headers (2 files).** The single-row path backtracked to the paren-aligned one-clause-per-line form only when `single_line_header and header_too_wide`. When a CLAUSE wrapped internally the first conjunct was false, the backtrack was skipped, and the header committed in the partial-break shape the standards' Anti-pattern section forbids — two clauses packed on the header line and one stranded beneath at an unrelated column. Escalation now fires when the header overflows OR any clause wrapped, which is the same rule as "if an argument breaks, the argument list breaks". **Tier 1 brace collapse (3 files).** The gate included `not _node_spans_multiple_rows(condition)`, on the reading that an author who spread a condition over rows wanted the Allman brace a multi-line condition triggers. But the emitter collapses that condition onto one line whenever it fits and emits it with no brace at all, so the gate contradicted its own behaviour — and being a source read it made the answer depend on layout about to be rewritten. The Tier 1 branch already decides by speculative emission measuring RENDERED widths, so the gate was redundant for correctness. It blocked 70 of 1,402 candidate sites; only 5 fit once collapsed, and those 5 are what the second pass already produced. Second-pass files: 6 to 1. The remaining one, `AbstractSchedulingService.java`, is layout-dependent but its mechanism is NOT established — a predicate trace diverges inside `_arg_list_takes_source_preserve_path`, but preservation fires zero times on that file, so that answer is discarded and cannot be the cause. An earlier draft of this entry asserted an ordering problem in the chain cascade; that was inferred from a comparison of two inputs that already agreed on output, which could not have detected what it was used to rule out. The claim is withdrawn rather than replaced. Also corrects figures this release had left stale: files reformatted against 0.6.0 (337, stated as both 322 and 336 in different places), net lines (+1,565), over-80 (1,571), and a per-category breakdown that summed above its own total. 797/797 pytest. Two fixtures and four unit tests, each verified to go red under the matching revert, the two revert sets disjoint.
CI's spellcheck flagged ten words introduced by this release's prose.
Per the project's cspell discipline these are reworded rather than
whitelisted: invented words get replaced with real ones and symbols
get renamed, rather than the dictionary being widened to accept them.
British to American, matching the rest of the repo: behaviour,
materialises, minimising, equalises. Coinages to real words: dedups
to de-duplicates, fixpoints to fixed points, unsplittable to unsplit.
"idempotently" is replaced by plain prose ("and the result is
idempotent"), which reads better anyway. And the test constant
MULTIROW_CONDITION is renamed WRAPPED_CONDITION, since "multirow" is
not a word.
Comments, docstrings and release notes only: 797/797 pytest, corpus
unchanged at 1571 over-80, 301 advisories, 6 to 1 second-pass.
The previous rewording pass missed these because it matched on word boundaries, and `_` is a word character — so `\bunsplittable\b` never matched inside `test_..._unsplittable_value`. cspell tokenizes on the underscore, and it also scans PATHS, which is how a fixture directory name slipped through. Renamed: `test_equalises_rather_than_packing` to `..._equalizes_...`; `test_still_reports_a_genuinely_unsplittable_value` to `test_still_reports_a_value_with_no_split_point`; `test_multirow_condition_*` to `test_wrapped_condition_*`; and the fixture `need_braces/23_multirow_source_condition_still_collapses` to `23_wrapped_source_condition_still_collapses`. Test and fixture names only: 797/797, corpus unchanged at 1571 over-80, 301 advisories, 1 second-pass file.
🤖 Claude Code ReviewCombined Code ReviewReview Part 1 of 3PR Code Review — Part 1 of 3Reviewing the diff shown so far ( Code Quality❌ Defect — new "LineLength ignorePattern" advisory-suppression is incomplete and inconsistently applied ( The actual checkstyle rule is: The new That third missing pattern ( Failure scenario: a field like Recommendation: either move the ✅ No commented-out code. All new comment blocks are prose rationale (design history, invariants), not disabled code. ✅ Meaningful names.
No ✅ CI change looks sound. The new Dependabot dedup fix looks correct — removing the second Documentation✅ CHANGELOG.md updated — extremely detailed 0.7.0 entry with before/after examples and measured corpus effects. ✅ New FAQs are well-formed markdown ( Note (not a defect): the 0.7.0 changelog entry is unusually long (over 1000 lines). Given how detailed Security✅ No hardcoded credentials, no Low-severity note: the corpus-gate job checks out and formats a third-party open-source repo's source as test input only (not executed) — negligible risk, flagging only per the checklist's supply-chain-awareness intent. Pending Parts 2–3: I have not yet seen Review Part 2 of 3I used the wrong output format for this task — the prompt asks for a checklist-style review, not the findings tool. Here's the proper review of Part 2 of the diff. Review — PR Diff (Part 2 of 3)Scope note: this part covers Code Quality
Testing
Documentation
Security
SummaryTwo minor findings, both non-functional:
No functional defects found in the wrap-engine logic reviewed; the flag-management mechanisms I traced ( Review Part 3 of 3Review — PR Diff (Part 3 of 3)Scope: this part covers the tail end of Code Quality
Testing
Documentation
Security
Combined Summary (Parts 1–3)
Everything else across all three parts — style, naming, idempotency/snapshot/restore correctness in the wrap engine, CI pin rationale, dependabot dedup, changelog/FAQ documentation, and security — checked out clean (✅). Item 1 is the one finding I'd treat as blocking before merge; items 2–4 are good follow-up cleanup but not release blockers. Automated code review analyzing defects and coding standards |
CI's automated review found this and rated it blocking; it was right. The release claims to have fixed noisy advisories, and had only half done it. `_LINE_LENGTH_EXEMPT_MARKERS` mirrored five of the eight `ignorePattern` alternatives in `checkstyle/senzing-checkstyle.xml`. The three omitted are not plain substrings and so could not live in a substring list: `^package.*`, `^import.*` and `static final.*<.*>`. They are now matched structurally in `_line_length_exempt`, the two anchored ones with `startswith` on the raw text since a `package` keyword inside a line is not a package declaration. More consequentially, `_line_length_exempt` had exactly ONE call site, in the javadoc path. Every wrap engine reaches the advisory channel through `_fire_wrap_overflow_advisory`, which never consulted it. It now excludes exempt lines from the width accounting, so the exemption is inherited by every site at once and an advisory whose only over-long lines are exempt does not fire at all. Corpus advisories 301 to 288. The exemption suppresses 257 sites but most were already deduplicated away, so thirteen is the net change. By rule, 250 suppressed sites are a `static final` constant whose generic type makes the declaration unbreakable, seven are URLs. Declaration advisories drop 19 to 12 and their reported widths narrow from 84-94 to 84-85, since the `static final` cases were the wide ones at 88 and 93 columns. Output is untouched: token streams identical in all 504 files and zero files differ, so this is advisory-channel only. Lines over 80 (1571), line count and convergence (1 second-pass, 0 third-pass) all unchanged. 797/797. Also from the same review: removed the dead `args_emit_column` parameter and the `name_node`/`name_text` locals that existed only to compute it, at both chain call sites. Its docstring justified it by the source-preserve gate being column-sensitive — that gate lost its column parameter when preservation was reduced to the comment and CSOFF cases. Rewrapped one run-on docstring line.
🤖 Claude Code ReviewCombined Code ReviewReview Part 1 of 3Review — Part 1 of 3I reviewed this against the repo as checked out at the merge commit ( Code Quality
TestingNot assessable from this chunk — no test files appear in Part 1's diff. Deferring to Parts 2/3 (the CHANGELOG references many new fixtures and unit tests; I'll check they exist and match the described coverage once visible). Documentation
Security
Still pendingPart 1 shows no Review Part 2 of 3Code Review — Part 2 of 3This chunk covers Code Quality
Testing
Documentation
Security
Review Part 3 of 3Code Review — Part 3 of 3Note: this chunk contains only test code (pytest tests for Code Quality
Testing
Documentation
Security
SummarySolid, well-targeted regression tests for previously-fixed idempotency/convergence bugs in the Java formatter. The one real concern: Automated code review analyzing defects and coding standards |
No correctness findings this round; CI confirmed the LineLength
exemption fix and noted that `startswith("package ")` is actually
MORE correct than the regex it mirrors, which would match an
identifier like `importantFlag` at line start.
Four things acted on:
**DRY.** The semicolon-reserve block — five lines plus a nine-line
comment — was duplicated verbatim five times across
`_emit_variable_declarator` and
`_emit_variable_declarator_with_array_rhs`. Extracted as an
`_extra_tail_reserve` context manager carrying the explanation once.
Verified output-neutral (token streams identical in all 504 files,
zero files differ) and verified the guard is not weakened: making
the single helper a no-op now fails five tests, where before the
same change had to be made in five places to be caught.
**`Emitter.snapshot()` docstring** enumerated ten field names for an
eleven-element tuple — `raw_rows_emitted` was missing. That docstring
is explicitly the contract other wrap-engine authors follow.
**`emit_p4_packed`'s missing per-argument reserve** is intentional
and now says so. Its sibling `emit_p4_multi_arg` can reserve because
each argument owns its line, so what follows is known; here every
argument shares one line and what follows argument N is the rest of
that line, unknown until emitted. The tier relies on the post-hoc
width check instead, which is sound — an overflowing candidate falls
to P3. Reserving only for the last argument, the one the `)` really
does follow, was implemented and measured at zero corpus change, so
it is documented rather than carried as speculative code.
**`actions/checkout`** was pinned at `v7.0.1` in the pytest job but
`v7.0.0` in both corpus-gate checkouts; aligned on `v7.0.1`.
Declined: CI called `TestSecondPassConvergence` redundant with
`test_fixture_golden`. Reverting either convergence fix fails the
unit tests as well as the fixtures, and the golden test checks
output-matches-expected rather than convergence-from-input, so they
are complementary. The maintenance-drift risk it was really pointing
at is already gone — those tests read the fixture inputs.
Ticketed rather than silently locked in: the deduped advisory
under-reports width by one (86 for an 87-column line). Pre-existing,
unchanged here, and now tracked as its own task so the test asserting
86 does not quietly become the specification.
797/797. Corpus unchanged: 1571 over-80, 288 advisories, 1
second-pass file, 0 third-pass.
🤖 Claude Code ReviewCombined Code ReviewReview Part 1 of 3All review work is complete — both background agents finished and I independently verified their two strongest findings by reading the actual source. Here's the full checklist review. Senzing Code Review — PR 0.7.0 (formatter release)Scope: Code Quality
Testing
Documentation
Security
SummaryOverall this is a well-tested, extensively documented release. Two real defects worth fixing before merge: the self-contradicting docstring in Review Part 2 of 3Code Review — Part 2 of 3Reviewing the Code Quality✅ Style conventions — Consistent with the file's existing (very verbose, rationale-heavy) documentation style. No formatting inconsistencies observed. ✅ No commented-out code — All large comment blocks are prose/rationale, not disabled code.
✅ DRY — This diff is largely a DRY improvement over the prior version:
Defects / edge cases — No confirmed logic bugs found in this chunk. Two items I could not fully verify because they depend on code outside this diff slice (the
N/A — CLAUDE.md: no Testing✅ Unit tests for new functions — ✅ Regression coverage — ✅ Golden fixture coverage — ~20 new/renamed fixtures under ✅ Test cleanup — Coverage >80% — Cannot verify a percentage from a diff alone; the breadth of new fixtures/tests strongly suggests good coverage of the new branches introduced. DocumentationN/A in this chunk — No README/CHANGELOG.md/markdown changes appear in this part of the diff; Inline comments for complex logic — ✅ Exceptionally thorough — every new predicate/tier ( Security✅ No hardcoded credentials, no sensitive logging, no license files — Scanned the diff text for N/A — input validation / error handling — This is a pure AST-transform code path operating on already-parsed source; no new external inputs, network calls, or user-facing error paths introduced. SummaryThis is a well-engineered, heavily self-documented change with strong test/fixture coverage for every new behavior. I found no confirmed defects in this chunk — only two low-confidence items that depend on the Review Part 3 of 3PR Code Review — Part 3 of 3Scope note: This fragment contains only Python test code (pytest test classes for Code Quality
Testing
Documentation
Security
SummaryThis chunk is test-only and high quality: thorough regression coverage for real formatter defects (off-by-one line-width overflow, reserve-computation oscillation, multi-pass non-convergence), with unusually good self-documentation of why each test exists. The only actionable item is the one flagged Automated code review analyzing defects and coding standards |
No confirmed defects this round. Three items raised; two acted on, one declined with evidence. **Renamed `emit_p4_packed` to `emit_p2b_packed`.** The function implements what the spec and its own docstring call priority 2b — it is tried between P2 and P3 — so the `p4` name was actively misleading and, as CI noted, a maintainer grepping for the P2 family would miss it. The docstring's "named for historical reasons" apology goes away with it. **De-duplicated the catch-parameter modifiers refusal.** The multi-type wrap engine and `_emit_catch_formal_parameter` each carried their own copy of the same `NotImplementedError`, so relaxing one when annotation support lands would silently leave the other in force. Now one `_refuse_catch_parameter_modifiers` helper called from both; a single copy of the message remains in the file. **Declined the multi-catch fixture-coverage finding.** CI reported that `multi_catch_wrap/` has no 4+ exception-type or deeply-nested case. It has both: `01_multi_catch_paren_align_when_inline_overflows` carries four types (`ClassNotFoundException | NoSuchMethodException | InvocationTargetException | IllegalAccessException`) and sits nested inside an `if` within a method. I wrote the suggested fixture before checking, found it duplicated fixture 01's shape with only the names changed, and removed it rather than add the redundant coverage CI flags elsewhere. `enhanced_for_wrap/` likewise already covers a nested generic iterable and an over-long header. Also verified, since CI raised it as low-confidence: `_anchor_escaped` is in `Emitter.__slots__`, in the `snapshot()` tuple and in `restore()`'s unpacking, so the speculative-emission contract does cover it. 797/797. Output untouched — token streams identical in all 504 files. Corpus unchanged: 1571 over-80, 288 advisories, 1 second-pass.
🤖 Claude Code ReviewCombined Code ReviewReview Part 1 of 3Senzing Code Review — 0.7.0 Formatter ReleaseReviewed via the local checklist ( Code Quality
Testing
Documentation
Security
CI/Build
Summary: two real, fixable documentation defects (the stale Review Part 2 of 3I'll present the review directly here instead of writing to file. Senzing Code Review — Part 2 of 3Scope of this chunk: the bulk of Code Quality
Testing
Documentation
Security
SummaryOne confirmed-but-harmless finding: a dead Review Part 3 of 3Code Review — PR Diff (Part 3 of 3)Note: I only have this final chunk of the diff (test additions to what appears to be Code Quality
Testing
Documentation
Security
SummaryNo defects introduced by this chunk; it's well-constructed regression testing with unusually good root-cause documentation. Only actionable note: the deliberately-preserved off-by-one warning bug should have a tracking reference so it isn't forgotten. I'd need parts 1–2 (or the full diff) to assess README/CHANGELOG/CLAUDE.md and overall coverage checklist items. Automated code review analyzing defects and coding standards |
Three ❌ findings, all real, plus four⚠️ items. No correctness defect. **Stale docstring claiming a bug this PR fixed.** `_emit_formal_parameters` still said receiver parameters "are NOT supported and, worse, are silently DROPPED... no emitter is registered for `receiver_parameter`" — while `_emit_receiver_parameter` exists, is registered, and `receiver_parameter` is kept by the filter a few lines below. As CI put it, a future reader could "fix" an already-fixed bug from this. Rewritten, and it now also records the padding carve-out. **`.claude/070_REMAINING_SCOPE.md` contradicted the CHANGELOG.** Its header said nothing was outstanding while its "Deferred to 0.8" list still named three items shipped inside 0.7.0. Split into what was folded in after the census was written and what genuinely remains — the latter being the one chain-with-lambda file whose mechanism is still unidentified. **`actions/setup-python` drift**: `v7.0.0` in the pytest job, `v6.2.0` in the corpus-gate job added by this same PR. Aligned on `v7.0.0`. **Dead clause removed.** The greedy dot-aligned chain tier guarded on `not chain_is_sole_arg and head is not None`, but `chain_is_sole_arg` is only ever assigned inside `if chain_is_positional_arg and head is None`, so it cannot be True when `head is not None`. Verified by measurement: removing it leaves output byte-identical. It read as though headless sole-argument chains were excluded from that tier, when they can never reach it. **P2's local patch is safe, and now says why.** CI flagged that P2 patches only the offending argument where P1 and P3 escalate the whole candidate, and could not construct a failure. Neither could I, and the reason is structural: the caller's two-line cap plus its width check mean a patched argument that still wraps spills to a second continuation line and the tier is rejected, while one that fits but overflows is rejected on width. Documented rather than given a fixture that cannot fail. **Accepted debt made visible.** The test asserting "max line width 86" now says explicitly that the assertion must move to 87 when the deduped-advisory off-by-one is fixed, and that it is asserted so the fix cannot land unnoticed. Tracked as its own task. **Declined for this release**: unifying the three near-identical member-iteration loops. A genuine simplification, and CI is right that this duplication class caused the pre-0.7.0 side-comment gap — but it touches the most-exercised emission path in a release that is otherwise converged and green. Tracked for early 0.8 with a byte-identical-output gate. Also: FAQ "Related" entries now use markdown links, matching the sibling FAQs. 797/797. Output untouched — token streams identical in all 504 files, zero files differ. Corpus: 1571 over-80, 288 advisories, 1 second-pass, 0 third-pass.
🤖 Claude Code ReviewCombined Code ReviewReview Part 1 of 3Verification complete. I've now personally confirmed the top findings by reading the actual source. Compiling the final consolidated review. Senzing Code Review — PR #50 (java-coding-standards, 0.7.0 formatter release)Reviewed against the actual repository state (this PR is already merged into the checked-out working tree: Top findings (ranked)1. ❌ ~130 lines of dead code in the argument-list source-preserve path, described in the CHANGELOG as active behavior. 2. ❌ Basic for (int i = 0; i < arr.length; i++)
{
process(arr[i]);
}(a short single-line header with an Allman-style body brace — a very ordinary shape in non-canonical input), 3. ❌ 4. ❌ 5. ❌ The new "convergence detector" script in the docs can never actually detect non-convergence. Medium/lower-confidence findings
Checklist summaryCode Quality
Testing
Documentation
Security
Review Part 2 of 3Code Review — Part 2 of 3Scope note: this segment covers Code Quality
Testing
Documentation
Security
SummaryThis chunk is a large but internally consistent refactor of the argument-list/method-chain wrap cascade, removing a documented source of non-idempotency (the old width-based source-preservation fallback) and replacing several ad hoc heuristics with structurally-defined predicates ( Review Part 3 of 3Code Review — Part 3 of 3Note on scope: This chunk contains no Code Quality
Testing
Documentation
Security
Overall: This is high-quality, well-justified regression-test code. No blocking issues found in this chunk; the one item worth a human's attention is confirming the pre-existing off-by-one bug (86 vs. 87) referenced in Automated code review analyzing defects and coding standards |
…lines Five findings, four in the formatter. The first is a genuine formatting defect that a golden file was enshrining. **Short `for` headers were exploded whenever the brace was Allman.** `_emit_for_statement` asked "was the source header multi-row?" using `body.start_point[0] != node.start_point[0]` — the BODY BRACE's row, which is true for every Allman-braced `for` no matter how short the header. A 36-column header came out as three paren-aligned clauses, and it self-perpetuated, since the reformatted header genuinely is multi-row. `assignment_wrap/02` had a golden locking in the wrong shape, which is how it survived review until now. The check now measures the header's own span; the Allman decision keeps the body-row test under its own name, `body_on_new_row`. **Removed ~124 unreachable lines** from the argument-list source-preserve path. After 0.7.0 narrowed preservation to comments and CSOFF, the branch re-tested exactly those two and returned, so the column-remap below could never run. Proven twice: tautological by construction, and by poisoning the block and formatting all 504 corpus files with zero hits. The release notes had described it as live behavior. **`array_creation_expression` added to `_COMPUTED_RECEIVER_TYPES`**, so `new int[computeSize()].length` is treated as a computed receiver rather than a name. **Field access advises when it declines.** Both silent exits committed a possibly-overflowing line without an advisory — the same gap this release closed for parameter lists and javadoc. Advisories 288 to 289: one previously-silent overflow now reports. **The trial checklist's convergence detector could not detect non-convergence.** It committed after every changed pass, so the tree was clean when the loop exited and the final check never fired — for exactly the oscillating file it exists to catch. Corpus: over-80 1571 unchanged, second-pass 1, third-pass 0, net vs 0.6.0 now +1,563 lines across the same 337 files. 797/797.
🤖 Claude Code ReviewCombined Code ReviewReview Part 1 of 3Code Review — 0.7.0 Formatting ReleaseReviewed via three parallel deep-dives across the full diff (~9,400 lines: CI/docs/CHANGELOG, the core Code Quality✅ Style conventions — Python style is consistent with the existing codebase; Markdown is consistent elsewhere except one paragraph noted below. ✅ No commented-out code — The very long inline comments are deliberate historical-rationale documentation (a consistent house style in this file), not dead code. ✅ Meaningful variable names — No issues found; names like ✅ DRY — ✅ Defects (formatter logic) — Two independent reviewers traced the highest-risk areas — ❌ CHANGELOG.md — stale summary figure contradicts the release's own final numbers (
❌ CHANGELOG.md — numeric contradiction: 288 vs 289 advisories ("A method chain's receiver reserve..." section): states "309 advisories against the 288 this release ships", but every other place in the same file states the shipped total as 289 ("Corpus advisories: 301 to 289"; "taking advisories to 289"). The same paragraph also separately cites "305 advisories" as the count "at the time it was measured" — three different numbers (288/289/305) appear for what should be a single tracked figure, with no explanation reconciling them. ❌ CHANGELOG.md — malformed paragraph wrapping (same "receiver reserve" section, the "The reserve is now ✅/ℹ️ Project CLAUDE.md config — Not applicable; no Testing✅ New predicates ( ℹ️ Minor — known bug pinned as an expected test value ( ✅ Edge cases — Not exercised: Documentation❌ See CHANGELOG findings above (stale "26 to 6" figure, 288/289 contradiction, malformed paragraph). ✅ Inline comments — Spot-checked against the actual code for the riskiest new logic (escape-scan fix, snapshot/restore, reserve arithmetic) and found accurate, not contradictory. ✅ New/changed Markdown FAQs ( ✅ Bash snippet correctness ( ℹ️ Minor inconsistency: the older "Gate 5" section (under "What changed in 0.5.0 → 0.5.1") was edited to say "Gate 5 — convergence" and duplicates the Security✅ No hardcoded credentials, unsafe eval/exec, path traversal, or logged sensitive data found anywhere across all three chunks. ✅ CRITICAL check — license files: searched the entire diff for ✅ Dependency pin changes ( SummaryThis is a very well-documented, internally cross-checked release, and the deep-dive into the actual wrap-engine logic (snapshot/restore discipline, tier symmetry, off-by-one fixes) held up under scrutiny — no functional defects found in Review Part 2 of 3Code Review — Part 2 of 3 (
|
CI review round 6. One real defect, in the fix I made for round 5. **`_emit_field_access` still shipped an over-long line in silence.** Round 5 added advisories to two of its three terminal commits. The third — the broken form itself, when breaking before the dot NARROWS the line without getting it under the limit — sits after the revert-to-inline branch, so the branch's advisory never covered it. Reproduced: an unsplittable receiver already over 80 on its own, as `return longCall().someField;`, emitted a 90-column line with no warning. The advisory now fires unconditionally after the commit, matching every other terminal commit point in the file; it is a no-op when the lines fit, so one call covers all three paths. Three tests added, verified to go red when the advisory is moved back inside the branch. My first attempt at that revert did NOT reproduce the bug — it early-returned inside the `if`, leaving the fall-through still covered — so the tests appeared to pass against a variant that was not actually the pre-fix shape. **Stale figures reconciled.** The release intro and the source-preservation FAQ both still said second-pass files fall "26 to 6"; the final number is 1. The receiver-reserve section claimed "309 advisories against the 288 this release ships" while every other place says 289 — and re-running that comparison now gives a much larger, less meaningful gap, because the LineLength-exemption and field-access changes moved what an advisory counts. It is now stated as +19 against the 305 baseline in place when it was measured, with that caveat attached. The same sentence was duplicated in `format_java.py` and is corrected there too. Also rewrapped a paragraph whose line breaks had gone irregular. 800/800. Corpus: over-80 1571, advisories 289, second-pass 1, third-pass 0.
🤖 Claude Code ReviewCombined Code ReviewReview Part 1 of 3Senzing Code Review — 0.7.0 formatting release (a8e4e06^1..a8e4e06^2)Reviewed via direct Code Quality
Testing
Documentation
Security
SummaryOne real defect (multi-declarator tail-reserve gap), otherwise an unusually well-scrutinized change set — this PR already absorbed five internal review rounds, and it shows: the state-machine plumbing ( Review Part 2 of 3Good — CHANGELOG.md is being maintained across these commits. Now let me finalize the review for this diff segment. PR Code Review — Part 2 of 3Scope note: This part covers the argument-list wrap cascade, field-access wrapping, method-chain wrap rules, variable-declarator RHS wrapping, and associated test fixtures/requirements-pin changes in Code Quality
Testing
Documentation
Security
Overall: This is a well-scoped, well-tested continuation of the 0.7.0 argument-list/chain-wrap rework. I did not find a genuine defect in this part after verifying the flag-lifecycle concerns against the actual Review Part 3 of 3I don't have write access to that file, so here's the Part 3 review directly. PR Code Review — Part 3 of 3Scope note: This part is the tail of one large addition hunk in Code Quality
Testing
Documentation
Security
Overall: Clean. This is regression-test scaffolding for fixes already verified correct in Parts 1/2, and every claim a test docstring makes about the underlying algorithm checks out against the actual Automated code review analyzing defects and coding standards |
Spellcheck failed on `cd42886`: "unsplittable" came back in the round-5 and round-6 comments, and inside the synthetic Java identifier the new field-access tests use. I had reworded it once already and reintroduced it while writing those fixes. Reworded to "unbreakable" per the project's cspell discipline — reword rather than widen the dictionary. The identifier is renamed `extremelyLongUnbreakableXMethodCall...`, one character padded so the rendered width is unchanged: these tests turn on whether a line lands over 80, so a shorter name would have quietly stopped exercising the case. Verified by re-running the revert experiment — moving the advisory back inside its branch still fails exactly the two field-access tests. 800/800. Comments and test identifiers only.
CI review round 7 found one real defect, reachable and reproduced.
The declaration semicolon fix raises the reserve by exactly one
around each declarator's value. That is right for the LAST
declarator, but a statement can hold several joined by `, `, and a
non-last declarator is followed by `, name` for each one still to
come before the `;`. So
int result = someCallThatFillsToColumnSeventyNine(), other;
committed its first value at column 79, measured `79 + 1 <= 80`, and
let `, other;` push the line to 82. The statement-level advisory
caught it on disk, so it was not silent — but it was avoidable: with
the real suffix charged the cascade breaks at `=` and the line comes
out at 75.
The reserve is now charged from the names of the declarators still to
come. Identifiers emit verbatim, so that width is a function of the
AST, not of layout — the property this release exists to protect. A
later declarator carrying its own initializer remains under-charged
by that initializer's width; strictly better than charging nothing,
and noted as the remaining gap.
Zero corpus effect: the style does not occur in the 504-file corpus,
which is why six review rounds missed it. Output byte-identical in
all 504 files, over-80 1571, advisories 289, second-pass 1.
Fixture `method_decl_wrap/07_multi_declarator_reserves_its_suffix`
goes red when the suffix reserve is neutralised.
801/801.
🤖 Claude Code ReviewCombined Code ReviewReview Part 1 of 3Code Review — Part 1 of 3I cross-checked the diff against the actual checked-out files (the working tree already reflects the merge, so line numbers below are from the real files) and ran static analysis on the trickiest new logic (multi-catch wrap, record header cascade, Code Quality
Testing
Documentation
Security
I'll continue with parts 2 and 3 to complete the Testing/coverage assessment (fixture content itself) and finish the defect scan across the rest of Review Part 2 of 3I've reviewed this diff (Part 2 of 3) covering changes to Code Quality
Defects found:
Testing
Documentation
Security
SummaryThe core logic changes (nested-call wrap rules, field-access dot-breaking, P2b-packed tier, removal of the fragile width-based source-preservation fallback and P1F factory-chain heuristic) are well-reasoned, thoroughly commented, and backed by targeted fixtures showing the actual before/after formatting output. The one gap worth addressing is test coverage for the non-computed-receiver overflow branch in Review Part 3 of 3Reviewing this chunk (part 3 of 3 — I don't have parts 1/2 in this session, so this covers only the tests shown above; they appear to be new pytest classes added to a Java formatter's test suite in Python). Code Quality
Testing
Documentation
Security
Overall: This chunk is test-only and is well-constructed — heavy edge-case coverage and unusually good documentation of why each test exists (bug history, invariants). Only findings are a cosmetic blank-line inconsistency and a note to confirm the pinned pre-existing off-by-one bug has a real tracking reference. No blocking issues found in this part of the diff. Automated code review analyzing defects and coding standards |
Folds in what I had ticketed as a follow-up. On reflection the deferral was poorly reasoned: I adopted CI's "worth a follow-up ticket" framing instead of costing the fix myself, and this is a correctness bug in an advisory THIS release introduced, with a test asserting the wrong value. `_fire_wrap_overflow_advisory` de-duplicates when a nested engine has already fired for the same span, keeping the inner advisory as the more actionable pointer. But the inner one measured before the outer construct wrote its trailing characters, so the surviving number could fall short of what checkstyle sees — a declaration whose `;` lands in column 87 reported 86. The dedup now carries the larger width across while keeping the inner line, column and remedy. `FormatterWarning` gained a `width` field so the comparison need not parse the number out of the prose. Not the cosmetic off-by-one it looked like: 48 corpus advisories change, and one had been reporting 82 for a 92-column line. Measured against the widest line in each file, exact matches go 51 to 85 and the count that over-state is unchanged at 44 — so this removes under-reporting without introducing over-reporting. That metric is a bound, not a per-construct measurement; the delta is the signal. Output byte-identical in all 504 files, advisory count still 289, over-80 1571, second-pass 1. The test that asserted 86 now asserts 87, and its accepted-debt note is gone. 801/801.
🤖 Claude Code ReviewCombined Code ReviewReview Part 1 of 3I've now reviewed the complete diff (all 3 parts). Here's the review. Senzing Code Review —
|
…sitives CI round 8. One documentation defect, one spellcheck failure, and three code concerns — all three of which turn out to be non-issues, now documented as such so they stop being re-raised. **Spellcheck** flagged "dedup", which I introduced in the previous commit's comment. Reworded to "de-duplication". **Stale fixture reference.** `.claude/070_REMAINING_SCOPE.md` cited `arg_list_wrap/18_arg0_wraps_but_paren_aligned_still_fits`; the fixture was renamed to `18_arg0_wraps_so_whole_list_breaks` during the release and the census was not updated. **Two flagged flag-leaks are covered by snapshot/restore**, and both sites now say so. `_emit_variable_declarator`'s `_anchor_escaped` reset looks like it could leak a stale True into the backtrack's re-emission, but `saved` is snapshotted BEFORE the reset and the flag is part of the snapshot tuple, so `restore(saved)` puts the incoming value back; the explicit restore is only needed on the commit path, which does not call `restore`. Likewise `emit_p2b_packed` sets `_arg_list_p4_fired` without saving it, but the caller snapshots immediately before invoking the tier — and an ACCEPTED candidate should leave it set, because P4 genuinely fired. An earlier review round had already verified the first of these independently; the comments are there to stop a third pass at it. The conditional fixture gap raised alongside those was contingent on the first concern being real, so it does not apply. Output byte-identical, 801/801, corpus unchanged.
🤖 Claude Code ReviewCombined Code ReviewReview Part 1 of 3Bash commands are being blocked by approval in this sandbox, so I'll proceed with a manual review based on careful reading of the diff (which is sufficient for this analysis). Here's my review of Part 1 of 3. Review — Part 1 of 3This part covers Code Quality
TestingNot assessable from this part — no test files are included in this diff chunk (the fixture directory changes referenced in the changelog, e.g. Documentation
Security
Summary so far: No blocking defects found in this part. One low-severity advisory-suppression edge case in Review Part 2 of 3Review — Part 2 of 3This part covers the back half of Code Quality
No correctness defects, security issues, or crash/edge-case gaps found in this part beyond the dead-variable nit above. Testing
Documentation
Security
SummaryOne nit: remove the unused Review Part 3 of 3Both fixtures exist and match the paths referenced in Review — Part 3 of 3This part covers the tail end of Code Quality
Testing
Documentation
Security
SummaryNo blocking issues in this final part. One minor DRY nit (duplicated node-finding Automated code review analyzing defects and coding standards |
CI round 9 reported no blocking issues. Its one flagged item was a "possible defect" it also identified as a faithful port, and checking it out confirms the latter reading. `_line_length_exempt`'s `static final` test is a plain scan over the rendered line, so it also matches `static final` inside a string literal or a comment. That is fidelity rather than sloppiness: the question the function answers is "will checkstyle skip this line", and checkstyle asks `static final.*<.*>` of the same text. Compared against checkstyle's own regex over eleven lines — including every "misclassification" the review raised — the two agree on all of them, zero mismatches. Being MORE precise would be the defect: the formatter would advise about lines the build ignores, which is the noise this check was added to remove. Rather than argue that in prose, the agreement is now pinned by `TestLineLengthExemptMatchesCheckstyle`, which asserts the two verdicts match line-for-line and additionally fails if the `ignorePattern` in `checkstyle/senzing-checkstyle.xml` drifts away from the copy the test compares against. Also removed an unused `effective_max` local in the block-body-lambda branch of the argument-list cascade. 803/803. Output byte-identical, corpus unchanged: over-80 1571, advisories 289, second-pass 1, third-pass 0.
🤖 Claude Code ReviewCombined Code ReviewReview Part 1 of 3Code Review — 0.7.0 release PRNo CRITICAL security findings. No Verdict: approve with follow-ups. This is an unusually well-evidenced release — every behavior change is corpus-measured, convergence work is real (26→1 second-pass files), ~280 lines of provably-dead code removed, backed by 235 golden fixtures + ~40 unit tests. Nothing blocks on formatter correctness, but two CI/config items below are worth verifying before merge. Code Quality
Testing✅ Fixtures for all cited golden files are in the diff (nested_call_wrap, record_header_wrap, multi_catch_wrap, etc.) — no claim/verification gap. Edge cases well covered. Documentation
Security✅ All clear. Two items worth verifying before merge
Review Part 2 of 3I don't have write permission for that file, so I'll present the review directly here instead (matching the format already used in Code Review — Part 2 of 3 (parameter/field-access/argument-list/method-chain wrap engine)No CRITICAL security findings. No Verdict: approve. This chunk covers the bulk of the wrap-engine rework: the parameter-list P3 alignment-vs-width tradeoff, a new Code Quality
Testing✅ Every behavior change has a matching new/renamed fixture: parameter alignment-vs-width ( Documentation✅ Inline comments are thorough and, where spot-checked, accurate to the code rather than aspirational. No README/CHANGELOG/API docs in this chunk's diff — see chunk 1 for repo-wide doc drift findings (stale Security✅ Clear — internal formatter logic and test fixtures only, no credentials/logging/license concerns. One process note: Review Part 3 of 3Code Review — Part 3 of 3 (test file changes)This chunk is entirely test code: removal of a handful of Code Quality
Testing
Documentation
Security
Automated code review analyzing defects and coding standards |
Round 10 found no formatter defects. Six items, all documentation staleness or test-code hygiene, all fixed. **README was describing an older repo.** It called `docs/java-coding-standards.md` "the canonical 912-line rules doc" when it is now 3,580 lines, and its FAQ inventory listed four files when there are eight — missing both this release's additions plus `consumer-trial-checklist` and `cspell-word-list-policy`. The line count is dropped rather than corrected: it was destined to go stale again, and it carries no information a reader needs. **A FAQ contradicted this PR's own dependency bump**, still pinning `tree-sitter==0.25.2` in prose while `requirements.txt` and `GRAMMAR_VERSION` moved to 0.26.0. **The fixture-pair count** in `format_java.py` said 234 against 235 on disk. **`_first_arg_list_of` was misnamed.** It promised the FIRST `argument_list` and returned all of them — and callers depend on that, since they assert over the whole list. Renamed `_arg_lists_of`, with the docstring saying what it returns. **Three copies of the same pre-order tree walk** collapsed onto one `_nodes_of_type` helper: one in that helper's caller and two more in `TestIsAnonymousClass`. A single local `visit` closure remains, the one inside the shared helper. Left alone deliberately: `CHANGELOG.md`'s "912-line standards doc" sits inside the `[0.1.0]` section, where it is accurate history. 803/803. Output byte-identical across all 504 corpus files; over-80 1571, advisories 289, second-pass 1, third-pass 0.
🤖 Claude Code ReviewCombined Code ReviewReview Part 1 of 3PR Code Review — 0.7.0 formatting releaseReviewed the full diff (143 files changed: Code Quality
Testing
Documentation
Security
SummaryNo blocking issues found. This is an unusually well-self-audited PR — the CHANGELOG itself documents the review process finding and fixing several defects (including one genuine infinite oscillation bug in the method-chain receiver reserve) before this diff was finalized, each backed by a named regression fixture. The Review Part 2 of 3Code Review — PR Diff (Part 2 of 3)Note: This is a review of a diff fragment ( Code Quality
Testing
Documentation
Security
SummaryThis fragment is a mature, heavily self-documented change (0.7.0 line) — nearly every non-obvious decision has an inline rationale, and almost every behavioral change ships with a fixture. The one item worth a second pair of eyes before merge is the Review Part 3 of 3I don't have write permission for that scratch file, so here's the review directly. Code Review — PR Diff (Part 3 of 3)Scope: This fragment is entirely Code Quality
Testing
Documentation
Security
SummaryNo blocking issues. The only nit is the triple blank line at Automated code review analyzing defects and coding standards |
CI review round 11 found no blocking issues. Its only actionable item was three blank lines before `TestFieldAccessCommitAndWarn` where PEP 8 wants two. Its one flagged defect is the third raise of the same `_anchor_escaped` claim — that the backtrack path re-emits the value without resetting the flag, leaking a stale True. It does not: the snapshot is taken BEFORE the reset and `_anchor_escaped` is part of the snapshot tuple, so `restore(saved)` on the backtrack path puts the incoming value back; the explicit `prev_escaped` restore exists only for the commit path, which never calls `restore`. That is already stated in a comment directly above the code, which even notes it has been misread before. Verified independently in an earlier round. No change. Also declined: extracting the shared `*_invalid_wrap` tracking from `emit_p1` / `emit_p2_greedy` / `emit_p3_paren_one_per_line`. Same reasoning as the member-iteration loops — a refactor of the most-exercised cascade in a release that is otherwise converged, where the value is future-proofing rather than any present defect. 803/803, output unchanged.
🤖 Claude Code ReviewCombined Code ReviewReview Part 1 of 3I'll give my assessment of Part 1 now, but flag upfront: this review will be incomplete until Parts 2 and 3 arrive — the diff cuts off mid-function in Part 1 ReviewCode Quality
Testing
Documentation
Security
I'll continue with Parts 2 and 3 once they arrive to complete the Testing section and validate the remainder of the Review Part 2 of 3Review — Part 2 of 3 (
|
2 participants
Formatting release, promoted from 0.6.1 to 0.7.0 because it adds normative
rules to
docs/java-coding-standards.md, removes two behaviours that documentdescribed, and reformats 324 of the 504 files in the four-repo consumer trial
corpus. Adopters should bump the pin on its own commit.
Measured against 0.6.0 (504 files: senzing-commons-java, sz-sdk-java, sz-sdk-java-grpc, data-mart-replicator)
725 tests pass on the pinned tree-sitter 0.26.0. That figure needs a consumer
checkout — without a corpus,
test_fuzz_corpus.pyskip-marks and the 210skipped parametrisations are exactly the AST-equivalence and idempotency
checks. The new
corpus-gateCI job supplies one.The theme: layout decisions must not read layout
Nine defects in this release came from a predicate consulting how the source
happened to be written, which the formatter then rewrites — so the answer
changed on the next pass. That includes the headline change (retiring source
preservation's width-based fallback) and five off-by-one errors of one family:
a width measured over a span that included something the construct did not
write, or excluded something it did.
New normative rules
withdrawn wherever a call is embedded in another expression.
(with every argument on onecontinuation line. Renumbered from an earlier "3b" that documented it as
running after priority 3 when the code runs it before.
not just 1 and 2.
forheader wrapping: break before:, Allman brace.has always required but the formatter never produced.
the
(, not the method name.Behaviours removed
path. Preservation now fires only where reflow would corrupt (interleaved
comments,
CSOFF).unreachable by design.
Review
Four review rounds. Rounds 3 and 4 each found a MUST-FIX regression introduced
by the round before, including one where a fix of mine was itself wrong in the
same way as the bug it fixed. Round 4 also overturned one of its own
clearances. Two fixes were rejected after measurement rather than reasoning:
suppressing an escalation for preserved arguments (it re-introduced
history-dependence and left a file oscillating), and balancing javadoc reflow
unconditionally (135 files of churn for no benefit).
Known open items, all pre-existing and tracked for 0.8
parse — the argument list is built from
is_namedchildren and tree-sitterexposes comments as named nodes. Byte-identical at 0.6.0.
int n = call(a, b).someField;emits 85 columns.
Foo this) are silently dropped.