daniellansun commented on PR #2718: URL: https://github.com/apache/groovy/pull/2718#issuecomment-5009629687
> Non-blocking nits (optional follow-ups, worth a PR comment) > > 1. insertionPointAfter adds the token's codepoint length to its start column — a multi-line anchor token (triple-quoted string/GString before a missing }) overshoots the caret. Message stays correct; caret only. Rare. > 2. fromCastPattern reports the earliest high-confidence cast site independent of where ANTLR actually failed — intentional and well-guarded, but in a multi-error file the location could point at a different construct. > 3. The broad catch (IndexOutOfBounds | IllegalArgumentException) in collectDefaultChannelTokens silently proceeds with partial tokens — reasonable for non-buffered streams (and there's a ThrowingTokenStream test), already commented. Thanks for the careful review and the approval — notes below on each optional nit. 1) Multi-line insertion point / caret overshoot Agreed. `insertionPointAfter` now reuses `PositionConfigureUtils.endPosition`, so a multi-line anchor (e.g. triple-quoted string before a missing `}`) puts the caret on the last line of that token, after its final content, instead of overshooting on the start line. Covered with unit tests and end-to-end cases in `SyntaxErrorTest`. Related: `lastNonEof` also skips trailing `NL` so EOF-based carets stay on the last meaningful line. 2) `fromCastPattern` reports the earliest high-confidence cast site Yes — intentional. With bail-on-error we stop at the first recognition failure, so the earliest incomplete cast is almost always the right one. Biasing toward ANTLR’s offending token was tried; it regressed good cases such as `println ((int 123)` (caret moved off the cast operand). Left as earliest-site, with the multi-error trade-off documented on the method. 3) Broad catch in `collectDefaultChannelTokens` Agreed it’s the right shape for non-buffered / partial streams. Comment tightened slightly, and the `IllegalArgumentException` branch is now exercised alongside the existing `ThrowingTokenStream` (`IndexOutOfBoundsException`) coverage. -- 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]
