daniellansun commented on PR #2724: URL: https://github.com/apache/groovy/pull/2724#issuecomment-5076818766
> Approved with some minor potential cleanup possible. AI read: > > In short: well-scoped, well-tested, default behavior preserved, opt-in semantics correct. > > Minor notes (none blocking): > > * Sonar reported 7 new minor issues (gate still passed) — worth a glance before or after merge, likely trivia. > * Theoretical edge: under recovery, a CompilationFailedException thrown without any error having been recorded would surface as silent success — every current throw path records first, so this is hypothetical, but a cheap belt-and-braces guard (if collector empty, rethrow) could be suggested in review comments if you want. Thanks for the review and the careful notes. 1) Sonar (7 minor): Addressed in a follow-up: assertSame for reference equality, single-throw assertThrows lambdas, assertion on recoveringStrategySyncDoesNotThrow, and the unused-recognizer hook on createFailedPredicateErrorMessage (kept for parity with the other create* hooks; documented + requireNonNull). The AstBuilder "brain method" INFO is pre-existing size on an unrelated visitor method and left alone. 2) Empty-collector + recovery → silent success: Agreed — cheap and worth doing. buildAST now returns the partial module only when errorRecovery && errorCollector.hasErrors(); otherwise the CFE is rethrown. Covered by regression tests in Groovy9192 (empty collector rethrows; recorded diagnostics still return the partial module; fail-fast unchanged). -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
