drccrd commented on PR #4005:
URL:
https://github.com/apache/incubator-kie-tools/pull/4005#issuecomment-5665734849
> ### 🔵 Needs a closer look
> Range-boundary handling, comment preservation, and statement-limited CLI
writes contain unresolved correctness issues.
>
> Review details
> ### Suppressed comments (6)
> **Previously missed (6)** — in code that hasn't changed since the last
review.
>
>
**packages/drools-lsp/drools-formatter/src/main/java/org/drools/formatter/DeclareFormatter.java:133**
>
> * Comments between enum arguments are outside each argument context, so
formatting each argument independently skips them; `BlockBuilder` then consumes
the constant through its stop token. Formatting `A(1, /* note */ 2)` therefore
deletes the comment. Include inter-argument hidden tokens in the row model or
preserve the constant verbatim when such comments occur.
>
**packages/drools-lsp/drools-formatter/src/main/java/org/drools/formatter/DeclareFormatter.java:158**
> * `ParserRuleContext.getText()` omits hidden-channel comments. Because the
block subsequently marks the whole field consumed, a comment inside an
initializer such as `value : int = 1 /* rationale */ + 2` is deleted
permanently. Build the initializer cell from its token span with the
comment-aware emission path, including comments around `=`.
>
**packages/drools-lsp/drools-formatter/src/main/java/org/drools/formatter/FormatCLI.java:460**
> * `splitLines` unconditionally invents a final newline before splicing.
Consequently, `--write --lines` on an earlier messy rule also adds a newline to
an untouched EOF when the original file had none, violating the command's
statement-limited scope. Preserve the original EOF state unless an applied edit
actually reaches the final line.
>
**packages/drools-lsp/drools-formatter/src/main/java/org/drools/formatter/RhsFormatter.java:121**
> * The buffered statement is not emitted while `parenDepth > 0`, yet a
comment is emitted immediately. For `foo(a, // note\n b);`, this moves `//
note` before the entire call rather than keeping it after `a`. Keep
in-parenthesis comments in the buffered representation and force an appropriate
multiline rendering so their source association is preserved.
>
**packages/drools-lsp/drools-formatter/src/main/java/org/drools/formatter/StatementFormatter.java:244**
> * Comments between a function signature and its opening brace are skipped.
`getInnerText` advances the cursor through the parameters, and `emitVerbatim`
then jumps directly to the body span, so a comment such as `function void f()
/* note */ {}` is lost. Emit hidden tokens up to the body before writing it
verbatim.
>
**packages/drools-lsp/drools-lsp-server/src/main/java/org/drools/lsp/server/DroolsLspDocumentService.java:472**
> * LSP ranges are end-exclusive, but this passes the end line to an
inclusive formatter range. A multi-line selection ending at character 0 on the
next rule therefore formats that unselected rule too. Decrement a character-0
end line for non-empty multi-line selections before calling `formatRange`.
>
> * **Files reviewed:** 34/34 changed files
> * **Comments generated:** 0 new
> * **Review effort level:** Balanced
For these minor points:
* `splitLines` unconditionally invents a final newline before splicing:
fixed in
[26cc89e](https://github.com/apache/incubator-kie-tools/pull/4005/commits/26cc89e8b66fb51846ca09445190edc9af445924)
* LSP ranges are end-exclusive, but this passes the end line to an
inclusive formatter range:
[1c8fcb0](https://github.com/apache/incubator-kie-tools/pull/4005/commits/1c8fcb0c151a4fa8cd58fa7207be3480c2b97077)
* Regarding the comment points - I don't think the scenarios are really
valid for drools code, there are other places comments can be put and make the
code more readable. But dropping the comments entirely was probably not the
best behavior, so I added a guard in
[94847ed](https://github.com/apache/incubator-kie-tools/pull/4005/commits/94847ed41583803800c5058845cf872b8c3ab9d7)
that will loudly refuse these scenarios with a reason.
When testing this I also ran up against the deprecated syntaxes in the new
parser which are still valid in the legacy parser - so also added a gate for
this in
[259a469](https://github.com/apache/incubator-kie-tools/pull/4005/commits/259a469904670c3a7d93e46b5d142da2a7c0ba23),
and will follow-up with another branch to flag these syntaxes in the linter,
as this could help users migrate during the deprecation window.
--
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]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
