You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@kie.apache.org by "drccrd (via GitHub)" <gi...@apache.org> on 2026/09/12 18:24:30 UTC
[PR] kie-tools#3709: [drl-vscode-extension] add a configurable DRL formatter [incubator-kie-tools]
drccrd opened a new pull request, #4005:
URL: https://github.com/apache/incubator-kie-tools/pull/4005
New drools-formatter module (engine + CLI), wired into the language server as document and range formatting with live drools.lsp.formatter settings, and documented in the extension README. Major style decisions are configurable.
Co-authored by Claude Fable 5.1
--
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: commits-unsubscribe@kie.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@kie.apache.org
For additional commands, e-mail: commits-help@kie.apache.org
Re: [PR] kie-tools#3709: [drl-vscode-extension] add a configurable DRL formatter [incubator-kie-tools]
Posted by "drccrd (via GitHub)" <gi...@apache.org>.
drccrd commented on code in PR #4005:
URL: https://github.com/apache/incubator-kie-tools/pull/4005#discussion_r4004133237
##########
packages/drools-lsp/drools-formatter/src/main/java/org/drools/formatter/LhsFormatter.java:
##########
@@ -0,0 +1,671 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.drools.formatter;
+
+import java.util.List;
+
+import org.antlr.v4.runtime.Token;
+import org.drools.drl.parser.antlr4.DRL10Lexer;
+import org.drools.drl.parser.antlr4.DRL10Parser;
+
+/**
+ * Formats a rule's conditions (the when block) and a query's body.
+ *
+ * <p>This class walks the CST with typed {@code visit*} methods:
+ * <ul>
+ * <li>{@code visitLhs} → {@code visitLhsExpression} dispatches on
+ * expression type (and/or/unary/enclosed).</li>
+ * <li>{@code visitLhsUnary} handles patterns, not/exists, accumulate,
+ * forall, eval, and groupby.</li>
+ * <li>{@code visitPatternBind} renders a pattern on one line if it fits
+ * within {@code options.lineLength()} and its constraint parens hold no line
+ * comment (a {@code //} comment would swallow the rest of a one-line
+ * rendering); otherwise delegates to {@code emitReflowedPattern} which
+ * puts each constraint on its own line — trailing comments attached —
+ * with the closing {@code )} aligned to the opening.</li>
+ * <li>Parentheses are padded — {@code Person( age > 18 )} — whatever the
+ * source did, an empty pair staying {@code ()}. The rule is enforced in
+ * three places because parens reach the output three ways: token-by-token
+ * emission ({@code needsSpaceBetween}), verbatim RHS code
+ * ({@code rhsNeedsSpace}), and string-built signatures
+ * ({@code parenthesized}).</li>
+ * <li>{@code visitAndDef} emits explicit {@code and} keywords between
+ * patterns in accumulate/groupby source via a {@code nextPatternPrefix}
+ * mechanism.</li>
+ * </ul>
+ */
+final class LhsFormatter {
+ private final Emitter e;
+
+ // Prefix to prepend to the next pattern (e.g. "and " between patterns)
+ private String nextPatternPrefix = "";
+
+ LhsFormatter(Emitter e) {
+ this.e = e;
+ }
+
+ void visitLhs(DRL10Parser.LhsContext ctx) {
+ e.emitHiddenTokensBefore(ctx);
+ e.emit(e.indent() + "when");
+ e.newline();
+ e.depth++;
+ for (DRL10Parser.LhsExpressionContext expr : ctx.lhsExpression()) {
+ visitLhsExpression(expr);
+ }
+ e.depth--;
+ }
+
+ void visitLhsExpression(DRL10Parser.LhsExpressionContext ctx) {
+ e.emitHiddenTokensBefore(ctx);
+ if (ctx instanceof DRL10Parser.LhsExpressionEnclosedContext enclosed) {
+ e.emit(e.indent() + consumePatternPrefix() + "(");
+ e.newline();
+ e.depth++;
+ visitLhsExpression(enclosed.lhsExpression());
+ e.depth--;
+ e.emit(e.indent() + ")");
+ e.newline();
+ } else if (ctx instanceof DRL10Parser.LhsOrContext orCtx) {
+ List<DRL10Parser.LhsExpressionContext> exprs = orCtx.lhsExpression();
+ for (int i = 0; i < exprs.size(); i++) {
+ if (i > 0) {
+ e.emit(e.indent() + "or");
+ e.newline();
+ }
+ visitLhsExpression(exprs.get(i));
+ }
+ } else if (ctx instanceof DRL10Parser.LhsAndContext andCtx) {
+ List<DRL10Parser.LhsExpressionContext> exprs = andCtx.lhsExpression();
+ for (int i = 0; i < exprs.size(); i++) {
+ if (i > 0) {
+ nextPatternPrefix = "and ";
+ }
+ visitLhsExpression(exprs.get(i));
+ }
+ } else if (ctx instanceof DRL10Parser.LhsUnarySingleContext unary) {
+ visitLhsUnary(unary.lhsUnary());
+ }
+ }
+
+ private void visitLhsUnary(DRL10Parser.LhsUnaryContext ctx) {
+ e.emitHiddenTokensBefore(ctx);
+ // Consume any pending prefix (e.g. "and ") for non-pattern nodes.
+ // visitPatternBind consumes it itself; other handlers need it here.
+ if (ctx.lhsPatternBind() != null) {
+ visitPatternBind(ctx.lhsPatternBind());
+ emitTrailingInvocations(ctx);
+ return;
+ }
+ String prefix = consumePatternPrefix();
+ if (ctx.lhsExists() != null) {
+ visitLhsExists(ctx.lhsExists(), prefix);
+ } else if (ctx.lhsNot() != null) {
+ visitLhsNot(ctx.lhsNot(), prefix);
+ } else if (ctx.lhsEval() != null) {
+ String inner = e.styledText(ctx.lhsEval().conditionalOrExpression());
+ e.emit(e.indent() + prefix
+ + (e.options.parenPadding() ? "eval( " + inner + " )" : "eval(" + inner + ")"));
+ e.newline();
+ } else if (ctx.lhsForall() != null) {
+ visitLhsForall(ctx.lhsForall());
+ } else if (ctx.lhsAccumulate() != null) {
+ visitLhsAccumulate(ctx.lhsAccumulate());
+ } else if (ctx.lhsGroupBy() != null) {
+ visitLhsGroupBy(ctx.lhsGroupBy());
+ } else if (ctx.lhsExpression() != null) {
+ // parenthesized lhsExpression
+ e.emit(e.indent() + prefix + "(");
+ e.newline();
+ e.depth++;
+ visitLhsExpression(ctx.lhsExpression());
+ e.depth--;
+ e.emit(e.indent() + ")");
+ e.newline();
+ } else if (ctx.conditionalBranch() != null) {
+ e.emit(e.indent() + prefix + e.styledText(ctx.conditionalBranch()));
+ e.newline();
+ }
+ // exists/not/eval and a parenthesised group can each carry a trailing
+ // invocation too; the pattern-bind path above appends its own.
+ emitTrailingInvocations(ctx);
+ }
+
+ /**
+ * The trailing consequence invocations of an {@code lhsUnary}, rendered for
+ * the end of the element's line.
+ *
+ * <p>The grammar hangs these off {@code lhsUnary} as siblings of the element
+ * they qualify — {@code lhsPatternBind consequenceInvocation*},
+ * {@code lhsExists namedConsequenceInvocation?} — so visiting only the
+ * element drops them. Dropping one is silent: {@code $p : Bar( age > 18 ) if
+ * ( $p.age > 65 ) do[senior]} loses its guard while the {@code then[senior]}
+ * block survives, leaving a labelled consequence that can never fire, and the
+ * output still re-parses, so the output-reparse net cannot see it.
+ */
+ private String trailingInvocations(DRL10Parser.LhsUnaryContext ctx) {
+ StringBuilder sb = new StringBuilder();
+ for (DRL10Parser.ConsequenceInvocationContext invocation : ctx.consequenceInvocation()) {
+ sb.append(' ').append(e.styledText(invocation));
+ }
+ if (ctx.namedConsequenceInvocation() != null) {
+ sb.append(' ').append(e.styledText(ctx.namedConsequenceInvocation()));
+ }
+ return sb.toString();
+ }
+
+ /**
+ * Attach an {@code lhsUnary}'s trailing invocations to the element just
+ * emitted. The element may have taken several lines (reflowed, or forced
+ * multi-line by a comment), so this appends to the emitted text rather than
+ * to a current-line buffer.
+ */
+ private void emitTrailingInvocations(DRL10Parser.LhsUnaryContext ctx) {
+ String trailing = trailingInvocations(ctx);
+ if (trailing.isEmpty()) {
+ return;
+ }
+ e.appendToLastLine(trailing);
+ e.newline();
+ }
+
+ private void visitLhsExists(DRL10Parser.LhsExistsContext ctx, String prefix) {
+ if (ctx.lhsPatternBind() != null) {
+ if (emitMultiLineIfCommented(ctx.lhsPatternBind(), prefix + "exists ")) {
+ return;
+ }
+ String patternText = formatPatternBind(ctx.lhsPatternBind());
+ String line = e.indent() + prefix + "exists " + patternText;
+ if (line.length() <= e.options.lineLength()) {
+ e.emit(line);
+ e.newline();
+ } else {
+ emitReflowedPattern(ctx.lhsPatternBind(), prefix + "exists ");
+ }
+ } else if (ctx.lhsExpression() != null) {
+ e.emit(e.indent() + prefix + "exists(");
+ e.newline();
+ e.depth++;
+ visitLhsExpression(ctx.lhsExpression());
+ e.depth--;
+ e.emit(e.indent() + ")");
+ e.newline();
+ }
+ }
+
+ private void visitLhsNot(DRL10Parser.LhsNotContext ctx, String prefix) {
+ if (ctx.lhsPatternBind() != null) {
+ if (emitMultiLineIfCommented(ctx.lhsPatternBind(), prefix + "not ")) {
+ return;
+ }
+ String patternText = formatPatternBind(ctx.lhsPatternBind());
+ String line = e.indent() + prefix + "not " + patternText;
+ if (line.length() <= e.options.lineLength()) {
+ e.emit(line);
+ e.newline();
+ } else {
+ emitReflowedPattern(ctx.lhsPatternBind(), prefix + "not ");
+ }
+ } else if (ctx.lhsExpression() != null) {
+ e.emit(e.indent() + prefix + "not(");
+ e.newline();
+ e.depth++;
+ visitLhsExpression(ctx.lhsExpression());
+ e.depth--;
+ e.emit(e.indent() + ")");
+ e.newline();
+ }
+ }
+
+ private void visitLhsForall(DRL10Parser.LhsForallContext ctx) {
+ e.emit(e.indent() + "forall(");
+ e.newline();
+ e.depth++;
+ for (DRL10Parser.LhsPatternBindContext pb : ctx.lhsPatternBind()) {
+ visitPatternBind(pb);
+ }
+ e.depth--;
+ e.emit(e.indent() + ")");
+ e.newline();
+ }
+
+ private void visitLhsAccumulate(DRL10Parser.LhsAccumulateContext ctx) {
+ String keyword = ctx.DRL_ACC() != null ? "acc" : "accumulate";
+ e.emit(e.indent() + keyword + "(");
+ e.newline();
+ e.depth++;
+ // The grammar accepts "," or ";" after the source pattern, but the DRL
+ // language reference documents inline accumulate as
+ // accumulate( <source pattern>; <functions> [;<constraints>] )
+ // so a source "," is normalized to the documented ";" form.
+ visitAndDef(ctx.lhsAndDef());
+ e.appendToLastLine(e.options.normalizeTerminators() ? ";" : sourceSeparator(ctx.lhsAndDef()));
+ e.newline();
+ // accumulate functions
+ List<DRL10Parser.AccumulateFunctionContext> funcs = ctx.accumulateFunction();
+ for (int i = 0; i < funcs.size(); i++) {
+ e.emit(e.indent() + e.styledText(funcs.get(i)));
Review Comment:
I actively decided for this behavior during development. I don't think supporting comments makes sense here, it would make the code less readable and a comment can just be placed before the pattern.
I don't think a change is needed here but maybe ensure it is documented.
--
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: commits-unsubscribe@kie.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@kie.apache.org
For additional commands, e-mail: commits-help@kie.apache.org
Re: [PR] kie-tools#3709: [drl-vscode-extension] add a configurable DRL formatter [incubator-kie-tools]
Posted by "drccrd (via GitHub)" <gi...@apache.org>.
drccrd commented on code in PR #4005:
URL: https://github.com/apache/incubator-kie-tools/pull/4005#discussion_r4004141097
##########
packages/drools-lsp/drools-formatter/src/main/java/org/drools/formatter/Emitter.java:
##########
@@ -0,0 +1,573 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.drools.formatter;
+
+import java.util.ArrayList;
+import java.util.List;
+
+import org.antlr.v4.runtime.CommonTokenStream;
+import org.antlr.v4.runtime.ParserRuleContext;
+import org.antlr.v4.runtime.Token;
+import org.antlr.v4.runtime.misc.Interval;
+import org.drools.drl.parser.antlr4.DRL10Lexer;
+import org.drools.drl.parser.antlr4.DRL10Parser;
+import org.drools.drl.parser.antlr4.DRL10ParserHelper;
+
+/**
+ * Output side of the formatter: the buffer, indentation depth, hidden-token and
+ * comment emission, {@code @formatter:off} regions, and the two text extractors
+ * ({@code styledText} normalizes spacing, {@code sourceSlice} is byte-faithful).
+ * Every area formatter writes through one instance of this.
+ *
+ * <p>Comments and blank lines from the hidden channel are emitted by
+ * {@code emitHiddenTokensBefore} / {@code emitTrailingHiddenTokens}, which
+ * walk the token stream between the last emitted index and the current node.
+ * Block comments are re-indented; line comments are preserved.
+ *
+ * <p>Emission is STYLE-NORMALIZING and COMMENT-PRESERVING, not byte-preserving:
+ * token content and comments always survive, while spacing and the sanctioned
+ * separator/terminator normalizations (statement {@code ;}, accumulate/groupby
+ * source separator {@code ,} → {@code ;}) come from the formatter, not the
+ * source.
+ * <ul>
+ * <li>{@code styledText} — the standard emission helper: visible tokens
+ * of a node (or token span) joined per the house spacing rules
+ * ({@code needsSpaceBetween}), block comments kept inline, hidden
+ * whitespace discarded.</li>
+ * <li>{@code sourceSlice} — exact original text of a node (or token span),
+ * including comments and inter-token whitespace. Reserved for the sites
+ * where verbatim IS the contract: frozen regions/{@code emitVerbatim},
+ * the unparsed-rule fallback, and the multi-line reflow guards that keep
+ * a line comment's newline.</li>
+ * <li>Both advance {@code lastEmittedTokenIndex} so hidden-token emission
+ * never double-prints what a span already contains.</li>
+ * <li>{@code singleLine} — collapses newline+indent runs to single spaces
+ * for one-line emission contexts.</li>
+ * </ul>
+ */
+final class Emitter {
+
+ final DRL10Parser parser;
+ final CommonTokenStream tokens;
+ final StringBuilder out = new StringBuilder();
+ final FormatterOptions options;
+ final TokenSpacing spacing;
+ int depth = 0;
+ int lastEmittedTokenIndex = -1;
+ int consecutiveNewlines = 0;
+
+ // Source-line ranges (1-based, inclusive) frozen by @formatter:off/on
+ // markers; top-level statements overlapping these are emitted verbatim.
+ // Populated once the token stream is filled, via computeSkipRegions().
+ List<int[]> skipRegions = List.of();
+
+ Emitter(String text, FormatterOptions options) {
+ parser = DRL10ParserHelper.createDrlParser(text);
+ tokens = (CommonTokenStream) parser.getTokenStream();
+ this.options = options;
+ this.spacing = new TokenSpacing(options);
+ }
+
+ String result() {
+ return out.toString();
+ }
+
+ // ── output helpers ────────────────────────────────────────────────────
+
+ String indent() {
+ return options.indentUnit().repeat(Math.max(0, depth));
+ }
+
+ String indent(int offset) {
+ return options.indentUnit().repeat(Math.max(0, depth + offset));
+ }
+
+ void emit(String text) {
+ out.append(text);
+ if (!text.isEmpty()) {
+ consecutiveNewlines = 0;
+ }
+ }
+
+ void newline() {
+ out.append('\n');
+ consecutiveNewlines++;
+ }
+
+ void blankLine() {
+ if (consecutiveNewlines < 2) {
+ newline();
+ }
+ }
+
+ /**
+ * Append text to the last line of output (before any trailing newline).
+ * Used to attach suffixes like "," to a pattern's closing ")".
+ */
+ void appendToLastLine(String suffix) {
+ // Strip trailing newline, append suffix. Caller must call newline() afterwards.
+ int len = out.length();
+ while (len > 0 && out.charAt(len - 1) == '\n') {
+ len--;
+ }
+ out.setLength(len);
+ out.append(suffix);
+ consecutiveNewlines = 0;
+ }
+
+ /**
+ * Emits a header line and its metadata items (annotations, attributes) per
+ * options.headerMetadata(), leaving depth one deeper than on entry — the
+ * body's level. Comments between items are emitted before the item they
+ * precede; under INLINE that places them on their own lines after the header.
+ */
+ void emitHeader(String header, List<? extends ParserRuleContext> items) {
+ FormatterOptions.HeaderMetadata mode = options.headerMetadata();
+ if (mode == FormatterOptions.HeaderMetadata.INLINE) {
+ StringBuilder line = new StringBuilder(header);
+ for (ParserRuleContext item : items) {
+ String text = styledText(item);
Review Comment:
Same as within an accumulate - I don't think inline comments between a rule name and metadata is something that should be supported.
However to align with the non-inline modes, comments are no longer dropped per [7a0ae31](https://github.com/apache/incubator-kie-tools/pull/4005/commits/7a0ae31db7f824ec07842939f240bccafd5cd490)
--
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: commits-unsubscribe@kie.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@kie.apache.org
For additional commands, e-mail: commits-help@kie.apache.org
Re: [PR] incubator-kie-tools#3709: [drl-vscode-extension] add a configurable DRL formatter [incubator-kie-tools]
Posted by "drccrd (via GitHub)" <gi...@apache.org>.
drccrd commented on code in PR #4005:
URL: https://github.com/apache/incubator-kie-tools/pull/4005#discussion_r4062337148
##########
packages/drools-lsp/drools-lsp-server/src/main/java/org/drools/lsp/server/DroolsLspServer.java:
##########
@@ -441,9 +463,90 @@ public CompletableFuture<InitializeResult> initialize(InitializeParams params) {
});
}
+ textService.setFormatterOptions(formatterOptionsOf(params.getInitializationOptions()));
+
return CompletableFuture.supplyAsync(() -> initializeResult);
}
+ /**
+ * Pulls {@code drools.lsp.formatter} through {@code workspace/configuration} and
+ * registers for an empty configuration change, the pattern LSP 3.17 prescribes:
+ * "If the server still needs to react to configuration changes (since the server
+ * caches the result of {@code workspace/configuration} requests) the server should
+ * register for an empty configuration change using the following registration
+ * pattern" (LSP 3.17, workspace/configuration).
+ */
+ @Override
+ public void initialized(InitializedParams params) {
+ pullFormatterOptions();
+ LanguageClient target = client;
+ if (!clientSupportsConfigurationRegistration || target == null) {
+ return;
+ }
+ Registration registration = new Registration("drools.lsp.didChangeConfiguration",
+ "workspace/didChangeConfiguration");
+ try {
+ target.registerCapability(new RegistrationParams(List.of(registration)))
+ .exceptionally(e -> {
+ logger.log(Level.WARNING, "Client refused to register for configuration "
+ + "changes — formatter settings will need a restart", e);
+ return null;
+ });
+ } catch (Exception e) {
+ logger.log(Level.WARNING, "Client does not implement client/registerCapability", e);
+ }
+ }
+
+ CompletableFuture<Void> pullFormatterOptions() {
+ LanguageClient target = client;
+ if (!clientProvidesConfiguration || target == null) {
+ return CompletableFuture.completedFuture(null);
+ }
+ ConfigurationItem item = new ConfigurationItem();
+ item.setSection("drools.lsp.formatter");
+ int generation = formatterPullGeneration.incrementAndGet();
+ try {
+ return target.configuration(new ConfigurationParams(List.of(item)))
+ .thenAccept(answer -> {
+ if (generation == formatterPullGeneration.get()) {
+ applyPulledFormatterOptions(answer);
Review Comment:
probably not high priority in real use. Fixed in [8de1f34](https://github.com/apache/incubator-kie-tools/pull/4005/commits/8de1f34b2580710dac074ea551c47dfdebab6115)
--
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: commits-unsubscribe@kie.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@kie.apache.org
For additional commands, e-mail: commits-help@kie.apache.org
Re: [PR] incubator-kie-tools#3709: [drl-vscode-extension] add a configurable DRL formatter [incubator-kie-tools]
Posted by "drccrd (via GitHub)" <gi...@apache.org>.
drccrd commented on PR #4005:
URL: https://github.com/apache/incubator-kie-tools/pull/4005#issuecomment-5867383785
@jomarko were you able to re-test with the fixes?
--
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: commits-unsubscribe@kie.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@kie.apache.org
For additional commands, e-mail: commits-help@kie.apache.org
Re: [PR] kie-tools#3709: [drl-vscode-extension] add a configurable DRL formatter [incubator-kie-tools]
Posted by "drccrd (via GitHub)" <gi...@apache.org>.
drccrd commented on PR #4005:
URL: https://github.com/apache/incubator-kie-tools/pull/4005#issuecomment-5666483051
All done on my end. I sent an email to the dev@kie.apache.org list but haven't seen it arrive yet.
--
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: commits-unsubscribe@kie.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@kie.apache.org
For additional commands, e-mail: commits-help@kie.apache.org
Re: [PR] incubator-kie-tools#3709: [drl-vscode-extension] add a configurable DRL formatter [incubator-kie-tools]
Posted by "drccrd (via GitHub)" <gi...@apache.org>.
drccrd commented on code in PR #4005:
URL: https://github.com/apache/incubator-kie-tools/pull/4005#discussion_r4062312773
##########
packages/drools-lsp/drools-formatter/src/main/java/org/drools/formatter/RhsFormatter.java:
##########
@@ -0,0 +1,517 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.drools.formatter;
+
+import java.util.ArrayList;
+import java.util.List;
+
+import org.antlr.v4.runtime.Token;
+import org.drools.drl.parser.antlr4.DRL10Parser;
+
+/**
+ * Formats consequences (then blocks): token-level, since the parser keeps them opaque.
+ *
+ * <p>The DRL parser treats {@code then} blocks as opaque text with individual
+ * tokens on a special RHS channel, so formatting works at the token level
+ * rather than the CST level:
+ * <ul>
+ * <li>{@code emitConsequenceBody} collects visible RHS tokens into
+ * per-statement lists, splitting only at newlines where paren depth is 0
+ * (so continuation lines within expanded calls stay together).</li>
+ * <li>{@code emitRhsStatement} tries the compact (single-line) form first.
+ * If it exceeds {@code options.lineLength()}, it delegates to
+ * {@code rhsEmitExpanded}.</li>
+ * <li>{@code rhsEmitExpanded} uses {@code rhsMarkExpanded} to decide which
+ * {@code (} groups need expansion: any group whose compact rendering
+ * (including trailing {@code );}) would exceed the line limit is marked.
+ * Marked groups get one argument per line with the closing {@code )}
+ * on its own line at the same indent as the opening.</li>
+ * </ul>
+ */
+final class RhsFormatter {
+ private final Emitter e;
+
+ // Extra depth added when a case/default label is emitted without an inline
+ // consequence (consequence follows on subsequent lines). Reset to 0 when
+ // the next case/default label or closing '}' is encountered.
+ private int caseLabelDepthBump = 0;
+
+ RhsFormatter(Emitter e) {
+ this.e = e;
+ }
+
+ void visitRhs(DRL10Parser.RhsContext ctx) {
+ e.emitHiddenTokensBefore(ctx);
+ e.emit(e.indent() + "then");
+ e.newline();
+ e.depth++;
+
+ emitConsequenceBody(ctx.consequenceBody());
+
+ for (DRL10Parser.NamedConsequenceContext nc : ctx.namedConsequence()) {
+ e.depth--;
+ // named consequence header: "then[name]"
+ String ncText = nc.RHS_NAMED_CONSEQUENCE_THEN().getText();
+ e.emit(e.indent() + ncText);
+ e.newline();
+ e.depth++;
+ emitConsequenceBody(nc.consequenceBody());
+ }
+
+ e.depth--;
+ }
+
+ private void emitConsequenceBody(DRL10Parser.ConsequenceBodyContext ctx) {
+ if (ctx == null || ctx.getChildCount() == 0) return;
+
+ int startIdx = ctx.getStart().getTokenIndex();
+ int stopIdx = ctx.getStop().getTokenIndex();
+
+ // Collect visible RHS tokens into statements. A statement boundary is
+ // a newline where paren depth is 0 (balanced). Newlines inside parens
+ // are continuation lines and belong to the same statement.
+ List<String> stmtTokens = new ArrayList<>();
+ boolean pendingBlankLine = false;
+ int parenDepth = 0;
+ int lastTokenLine = -1;
+
+ for (int i = startIdx; i <= stopIdx; i++) {
+ Token t = e.tokens.get(i);
+ String text = t.getText();
+
+ if (t.getChannel() == Token.HIDDEN_CHANNEL) {
+ if (text.contains("\n") && parenDepth == 0) {
+ if (!stmtTokens.isEmpty()) {
+ emitRhsStatementInBlock(stmtTokens);
+ stmtTokens.clear();
+ }
+ long nlCount = text.chars().filter(c -> c == '\n').count();
+ if (nlCount > 1) pendingBlankLine = true;
+ }
+ // Newlines inside parens are ignored (tokens collected into same statement)
+ continue;
+ }
+
+ if (Emitter.isComment(t)) {
+ // A comment sharing its source line with the previous token is a
+ // trailing comment (e.g. "insert( new Foo() ); // audit note") —
+ // flush the statement it trails, then reattach it to that line
+ // instead of dropping it onto a fresh one.
+ boolean trailsPreviousToken = t.getLine() == lastTokenLine && parenDepth == 0;
+ if (!stmtTokens.isEmpty() && parenDepth == 0) {
+ emitRhsStatementInBlock(stmtTokens);
+ stmtTokens.clear();
+ }
+ if (trailsPreviousToken) {
+ e.appendToLastLine(" " + text.trim());
+ e.newline();
+ } else {
+ if (pendingBlankLine) { e.blankLine(); pendingBlankLine = false; }
+ e.emit(e.indent() + text.trim());
+ e.newline();
+ }
+ continue;
+ }
+
+ // Track paren depth for statement boundary detection.
+ // Only count () — curly braces are block delimiters (switch, if, for)
+ // and should not suppress newline-based statement splitting.
+ if (text.equals("(")) parenDepth++;
+ else if (text.equals(")")) parenDepth = Math.max(0, parenDepth - 1);
+
+ // case/default keywords at top level act as statement boundaries so
+ // that multiple cases concatenated on one line are split correctly
+ // (e.g. badly-formatted input: "break; case 2: ...").
+ if (isCaseLabelStart(text) && parenDepth == 0 && !stmtTokens.isEmpty()) {
+ emitRhsStatementInBlock(stmtTokens);
+ stmtTokens.clear();
+ }
+
+ if (pendingBlankLine) { e.blankLine(); pendingBlankLine = false; }
+ stmtTokens.add(text);
+ lastTokenLine = t.getLine();
+ }
+ if (!stmtTokens.isEmpty()) {
+ emitRhsStatementInBlock(stmtTokens);
+ }
+
+ e.lastEmittedTokenIndex = Math.max(e.lastEmittedTokenIndex, stopIdx);
+ }
+
+ /**
+ * Wrapper around {@link #emitRhsStatement} that manages brace-depth and
+ * case/default label splitting so that switch blocks are formatted as:
+ * <pre>
+ * switch( x )
+ * {
+ * case 1:
+ * value = ...; break;
+ * default:
+ * value = ...;
+ * }
+ * </pre>
+ * Rules:
+ * <ul>
+ * <li>Statement starting with {@code }} → decrement depth before emit.</li>
+ * <li>Statement ending with {@code {} → increment depth after emit.</li>
+ * <li>Statement starting with {@code case} or {@code default} → split at
+ * the first top-level {@code :}, emit label at current depth, bump
+ * depth by 1, emit consequence, restore depth.</li>
+ * </ul>
+ */
+ private void emitRhsStatementInBlock(List<String> toks) {
+ if (toks.isEmpty()) return;
+
+ String first = toks.get(0);
+ String last = toks.get(toks.size() - 1);
+
+ // Closing brace: undo any case-consequence depth bump, then decrease brace indent.
+ if (first.equals("}")) {
+ e.depth -= caseLabelDepthBump;
+ caseLabelDepthBump = 0;
+ e.depth--;
Review Comment:
fixed with [00e196e](https://github.com/apache/incubator-kie-tools/pull/4005/commits/00e196ef32b01260c4fabd595023f09be3539043)
--
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: commits-unsubscribe@kie.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@kie.apache.org
For additional commands, e-mail: commits-help@kie.apache.org
Re: [PR] kie-tools#3709: [drl-vscode-extension] add a configurable DRL formatter [incubator-kie-tools]
Posted by "drccrd (via GitHub)" <gi...@apache.org>.
drccrd commented on code in PR #4005:
URL: https://github.com/apache/incubator-kie-tools/pull/4005#discussion_r4004133237
##########
packages/drools-lsp/drools-formatter/src/main/java/org/drools/formatter/LhsFormatter.java:
##########
@@ -0,0 +1,671 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.drools.formatter;
+
+import java.util.List;
+
+import org.antlr.v4.runtime.Token;
+import org.drools.drl.parser.antlr4.DRL10Lexer;
+import org.drools.drl.parser.antlr4.DRL10Parser;
+
+/**
+ * Formats a rule's conditions (the when block) and a query's body.
+ *
+ * <p>This class walks the CST with typed {@code visit*} methods:
+ * <ul>
+ * <li>{@code visitLhs} → {@code visitLhsExpression} dispatches on
+ * expression type (and/or/unary/enclosed).</li>
+ * <li>{@code visitLhsUnary} handles patterns, not/exists, accumulate,
+ * forall, eval, and groupby.</li>
+ * <li>{@code visitPatternBind} renders a pattern on one line if it fits
+ * within {@code options.lineLength()} and its constraint parens hold no line
+ * comment (a {@code //} comment would swallow the rest of a one-line
+ * rendering); otherwise delegates to {@code emitReflowedPattern} which
+ * puts each constraint on its own line — trailing comments attached —
+ * with the closing {@code )} aligned to the opening.</li>
+ * <li>Parentheses are padded — {@code Person( age > 18 )} — whatever the
+ * source did, an empty pair staying {@code ()}. The rule is enforced in
+ * three places because parens reach the output three ways: token-by-token
+ * emission ({@code needsSpaceBetween}), verbatim RHS code
+ * ({@code rhsNeedsSpace}), and string-built signatures
+ * ({@code parenthesized}).</li>
+ * <li>{@code visitAndDef} emits explicit {@code and} keywords between
+ * patterns in accumulate/groupby source via a {@code nextPatternPrefix}
+ * mechanism.</li>
+ * </ul>
+ */
+final class LhsFormatter {
+ private final Emitter e;
+
+ // Prefix to prepend to the next pattern (e.g. "and " between patterns)
+ private String nextPatternPrefix = "";
+
+ LhsFormatter(Emitter e) {
+ this.e = e;
+ }
+
+ void visitLhs(DRL10Parser.LhsContext ctx) {
+ e.emitHiddenTokensBefore(ctx);
+ e.emit(e.indent() + "when");
+ e.newline();
+ e.depth++;
+ for (DRL10Parser.LhsExpressionContext expr : ctx.lhsExpression()) {
+ visitLhsExpression(expr);
+ }
+ e.depth--;
+ }
+
+ void visitLhsExpression(DRL10Parser.LhsExpressionContext ctx) {
+ e.emitHiddenTokensBefore(ctx);
+ if (ctx instanceof DRL10Parser.LhsExpressionEnclosedContext enclosed) {
+ e.emit(e.indent() + consumePatternPrefix() + "(");
+ e.newline();
+ e.depth++;
+ visitLhsExpression(enclosed.lhsExpression());
+ e.depth--;
+ e.emit(e.indent() + ")");
+ e.newline();
+ } else if (ctx instanceof DRL10Parser.LhsOrContext orCtx) {
+ List<DRL10Parser.LhsExpressionContext> exprs = orCtx.lhsExpression();
+ for (int i = 0; i < exprs.size(); i++) {
+ if (i > 0) {
+ e.emit(e.indent() + "or");
+ e.newline();
+ }
+ visitLhsExpression(exprs.get(i));
+ }
+ } else if (ctx instanceof DRL10Parser.LhsAndContext andCtx) {
+ List<DRL10Parser.LhsExpressionContext> exprs = andCtx.lhsExpression();
+ for (int i = 0; i < exprs.size(); i++) {
+ if (i > 0) {
+ nextPatternPrefix = "and ";
+ }
+ visitLhsExpression(exprs.get(i));
+ }
+ } else if (ctx instanceof DRL10Parser.LhsUnarySingleContext unary) {
+ visitLhsUnary(unary.lhsUnary());
+ }
+ }
+
+ private void visitLhsUnary(DRL10Parser.LhsUnaryContext ctx) {
+ e.emitHiddenTokensBefore(ctx);
+ // Consume any pending prefix (e.g. "and ") for non-pattern nodes.
+ // visitPatternBind consumes it itself; other handlers need it here.
+ if (ctx.lhsPatternBind() != null) {
+ visitPatternBind(ctx.lhsPatternBind());
+ emitTrailingInvocations(ctx);
+ return;
+ }
+ String prefix = consumePatternPrefix();
+ if (ctx.lhsExists() != null) {
+ visitLhsExists(ctx.lhsExists(), prefix);
+ } else if (ctx.lhsNot() != null) {
+ visitLhsNot(ctx.lhsNot(), prefix);
+ } else if (ctx.lhsEval() != null) {
+ String inner = e.styledText(ctx.lhsEval().conditionalOrExpression());
+ e.emit(e.indent() + prefix
+ + (e.options.parenPadding() ? "eval( " + inner + " )" : "eval(" + inner + ")"));
+ e.newline();
+ } else if (ctx.lhsForall() != null) {
+ visitLhsForall(ctx.lhsForall());
+ } else if (ctx.lhsAccumulate() != null) {
+ visitLhsAccumulate(ctx.lhsAccumulate());
+ } else if (ctx.lhsGroupBy() != null) {
+ visitLhsGroupBy(ctx.lhsGroupBy());
+ } else if (ctx.lhsExpression() != null) {
+ // parenthesized lhsExpression
+ e.emit(e.indent() + prefix + "(");
+ e.newline();
+ e.depth++;
+ visitLhsExpression(ctx.lhsExpression());
+ e.depth--;
+ e.emit(e.indent() + ")");
+ e.newline();
+ } else if (ctx.conditionalBranch() != null) {
+ e.emit(e.indent() + prefix + e.styledText(ctx.conditionalBranch()));
+ e.newline();
+ }
+ // exists/not/eval and a parenthesised group can each carry a trailing
+ // invocation too; the pattern-bind path above appends its own.
+ emitTrailingInvocations(ctx);
+ }
+
+ /**
+ * The trailing consequence invocations of an {@code lhsUnary}, rendered for
+ * the end of the element's line.
+ *
+ * <p>The grammar hangs these off {@code lhsUnary} as siblings of the element
+ * they qualify — {@code lhsPatternBind consequenceInvocation*},
+ * {@code lhsExists namedConsequenceInvocation?} — so visiting only the
+ * element drops them. Dropping one is silent: {@code $p : Bar( age > 18 ) if
+ * ( $p.age > 65 ) do[senior]} loses its guard while the {@code then[senior]}
+ * block survives, leaving a labelled consequence that can never fire, and the
+ * output still re-parses, so the output-reparse net cannot see it.
+ */
+ private String trailingInvocations(DRL10Parser.LhsUnaryContext ctx) {
+ StringBuilder sb = new StringBuilder();
+ for (DRL10Parser.ConsequenceInvocationContext invocation : ctx.consequenceInvocation()) {
+ sb.append(' ').append(e.styledText(invocation));
+ }
+ if (ctx.namedConsequenceInvocation() != null) {
+ sb.append(' ').append(e.styledText(ctx.namedConsequenceInvocation()));
+ }
+ return sb.toString();
+ }
+
+ /**
+ * Attach an {@code lhsUnary}'s trailing invocations to the element just
+ * emitted. The element may have taken several lines (reflowed, or forced
+ * multi-line by a comment), so this appends to the emitted text rather than
+ * to a current-line buffer.
+ */
+ private void emitTrailingInvocations(DRL10Parser.LhsUnaryContext ctx) {
+ String trailing = trailingInvocations(ctx);
+ if (trailing.isEmpty()) {
+ return;
+ }
+ e.appendToLastLine(trailing);
+ e.newline();
+ }
+
+ private void visitLhsExists(DRL10Parser.LhsExistsContext ctx, String prefix) {
+ if (ctx.lhsPatternBind() != null) {
+ if (emitMultiLineIfCommented(ctx.lhsPatternBind(), prefix + "exists ")) {
+ return;
+ }
+ String patternText = formatPatternBind(ctx.lhsPatternBind());
+ String line = e.indent() + prefix + "exists " + patternText;
+ if (line.length() <= e.options.lineLength()) {
+ e.emit(line);
+ e.newline();
+ } else {
+ emitReflowedPattern(ctx.lhsPatternBind(), prefix + "exists ");
+ }
+ } else if (ctx.lhsExpression() != null) {
+ e.emit(e.indent() + prefix + "exists(");
+ e.newline();
+ e.depth++;
+ visitLhsExpression(ctx.lhsExpression());
+ e.depth--;
+ e.emit(e.indent() + ")");
+ e.newline();
+ }
+ }
+
+ private void visitLhsNot(DRL10Parser.LhsNotContext ctx, String prefix) {
+ if (ctx.lhsPatternBind() != null) {
+ if (emitMultiLineIfCommented(ctx.lhsPatternBind(), prefix + "not ")) {
+ return;
+ }
+ String patternText = formatPatternBind(ctx.lhsPatternBind());
+ String line = e.indent() + prefix + "not " + patternText;
+ if (line.length() <= e.options.lineLength()) {
+ e.emit(line);
+ e.newline();
+ } else {
+ emitReflowedPattern(ctx.lhsPatternBind(), prefix + "not ");
+ }
+ } else if (ctx.lhsExpression() != null) {
+ e.emit(e.indent() + prefix + "not(");
+ e.newline();
+ e.depth++;
+ visitLhsExpression(ctx.lhsExpression());
+ e.depth--;
+ e.emit(e.indent() + ")");
+ e.newline();
+ }
+ }
+
+ private void visitLhsForall(DRL10Parser.LhsForallContext ctx) {
+ e.emit(e.indent() + "forall(");
+ e.newline();
+ e.depth++;
+ for (DRL10Parser.LhsPatternBindContext pb : ctx.lhsPatternBind()) {
+ visitPatternBind(pb);
+ }
+ e.depth--;
+ e.emit(e.indent() + ")");
+ e.newline();
+ }
+
+ private void visitLhsAccumulate(DRL10Parser.LhsAccumulateContext ctx) {
+ String keyword = ctx.DRL_ACC() != null ? "acc" : "accumulate";
+ e.emit(e.indent() + keyword + "(");
+ e.newline();
+ e.depth++;
+ // The grammar accepts "," or ";" after the source pattern, but the DRL
+ // language reference documents inline accumulate as
+ // accumulate( <source pattern>; <functions> [;<constraints>] )
+ // so a source "," is normalized to the documented ";" form.
+ visitAndDef(ctx.lhsAndDef());
+ e.appendToLastLine(e.options.normalizeTerminators() ? ";" : sourceSeparator(ctx.lhsAndDef()));
+ e.newline();
+ // accumulate functions
+ List<DRL10Parser.AccumulateFunctionContext> funcs = ctx.accumulateFunction();
+ for (int i = 0; i < funcs.size(); i++) {
+ e.emit(e.indent() + e.styledText(funcs.get(i)));
Review Comment:
I actively decided for this behavior during development. I don't think supporting comments makes sense here, it would make the code less readable and a comment can just be placed before the pattern.
I don't think a change is needed here but maybe ensure it is documented.
-> https://github.com/apache/incubator-kie-tools/pull/4005/changes#diff-7d8703ff9179fa3c7d45c4747bd4fc7526a14b595f903a1b444260f0573d0916R191
--
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: commits-unsubscribe@kie.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@kie.apache.org
For additional commands, e-mail: commits-help@kie.apache.org
Re: [PR] incubator-kie-tools#3709: [drl-vscode-extension] add a configurable DRL formatter [incubator-kie-tools]
Posted by "tkobayas (via GitHub)" <gi...@apache.org>.
tkobayas commented on PR #4005:
URL: https://github.com/apache/incubator-kie-tools/pull/4005#issuecomment-5675178146
@drccrd I see the mail has arrived :) https://lists.apache.org/thread/xvjsstwftbrmyxhl49frwl3rky9xnzlj
--
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: commits-unsubscribe@kie.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@kie.apache.org
For additional commands, e-mail: commits-help@kie.apache.org
Re: [PR] incubator-kie-tools#3709: [drl-vscode-extension] add a configurable DRL formatter [incubator-kie-tools]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #4005:
URL: https://github.com/apache/incubator-kie-tools/pull/4005#discussion_r4059374663
##########
packages/drools-lsp/drools-lsp-server/src/main/java/org/drools/lsp/server/DroolsLspServer.java:
##########
@@ -441,9 +463,90 @@ public CompletableFuture<InitializeResult> initialize(InitializeParams params) {
});
}
+ textService.setFormatterOptions(formatterOptionsOf(params.getInitializationOptions()));
+
return CompletableFuture.supplyAsync(() -> initializeResult);
}
+ /**
+ * Pulls {@code drools.lsp.formatter} through {@code workspace/configuration} and
+ * registers for an empty configuration change, the pattern LSP 3.17 prescribes:
+ * "If the server still needs to react to configuration changes (since the server
+ * caches the result of {@code workspace/configuration} requests) the server should
+ * register for an empty configuration change using the following registration
+ * pattern" (LSP 3.17, workspace/configuration).
+ */
+ @Override
+ public void initialized(InitializedParams params) {
+ pullFormatterOptions();
+ LanguageClient target = client;
+ if (!clientSupportsConfigurationRegistration || target == null) {
+ return;
+ }
+ Registration registration = new Registration("drools.lsp.didChangeConfiguration",
+ "workspace/didChangeConfiguration");
+ try {
+ target.registerCapability(new RegistrationParams(List.of(registration)))
+ .exceptionally(e -> {
+ logger.log(Level.WARNING, "Client refused to register for configuration "
+ + "changes — formatter settings will need a restart", e);
+ return null;
+ });
+ } catch (Exception e) {
+ logger.log(Level.WARNING, "Client does not implement client/registerCapability", e);
+ }
+ }
+
+ CompletableFuture<Void> pullFormatterOptions() {
+ LanguageClient target = client;
+ if (!clientProvidesConfiguration || target == null) {
+ return CompletableFuture.completedFuture(null);
+ }
+ ConfigurationItem item = new ConfigurationItem();
+ item.setSection("drools.lsp.formatter");
+ int generation = formatterPullGeneration.incrementAndGet();
+ try {
+ return target.configuration(new ConfigurationParams(List.of(item)))
+ .thenAccept(answer -> {
+ if (generation == formatterPullGeneration.get()) {
+ applyPulledFormatterOptions(answer);
Review Comment:
The generation check and option update are not atomic. An older callback can pass this check, pause while a newer request increments the generation and applies its answer, then resume and overwrite it with stale settings. Protect generation increments and this check-and-apply operation with the same lock, or serialize configuration pulls.
##########
packages/drools-lsp/drools-formatter/src/main/java/org/drools/formatter/LhsFormatter.java:
##########
@@ -0,0 +1,723 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.drools.formatter;
+
+import java.util.List;
+
+import org.antlr.v4.runtime.Token;
+import org.drools.drl.parser.antlr4.DRL10Lexer;
+import org.drools.drl.parser.antlr4.DRL10Parser;
+
+/**
+ * Formats a rule's conditions (the when block) and a query's body.
+ *
+ * <p>This class walks the CST with typed {@code visit*} methods:
+ * <ul>
+ * <li>{@code visitLhs} → {@code visitLhsExpression} dispatches on
+ * expression type (and/or/unary/enclosed).</li>
+ * <li>{@code visitLhsUnary} handles patterns, not/exists, accumulate,
+ * forall, eval, and groupby.</li>
+ * <li>{@code visitPatternBind} renders a pattern on one line if it fits
+ * within {@code options.lineLength()} and its constraint parens hold no line
+ * comment (a {@code //} comment would swallow the rest of a one-line
+ * rendering); otherwise delegates to {@code emitReflowedPattern} which
+ * puts each constraint on its own line — trailing comments attached —
+ * with the closing {@code )} aligned to the opening.</li>
+ * <li>Parentheses are padded — {@code Person( age > 18 )} — whatever the
+ * source did, an empty pair staying {@code ()}. The rule is enforced in
+ * three places because parens reach the output three ways: token-by-token
+ * emission ({@code needsSpaceBetween}), verbatim RHS code
+ * ({@code rhsNeedsSpace}), and string-built signatures
+ * ({@code parenthesized}).</li>
+ * <li>{@code visitAndDef} emits explicit {@code and} keywords between
+ * patterns in accumulate/groupby source via a {@code nextPatternPrefix}
+ * mechanism.</li>
+ * </ul>
+ */
+final class LhsFormatter {
+ private final Emitter e;
+
+ // Prefix to prepend to the next pattern (e.g. "and " between patterns)
+ private String nextPatternPrefix = "";
+
+ LhsFormatter(Emitter e) {
+ this.e = e;
+ }
+
+ void visitLhs(DRL10Parser.LhsContext ctx) {
+ e.emitHiddenTokensBefore(ctx);
+ e.emit(e.indent() + "when");
+ e.newline();
+ e.depth++;
+ for (DRL10Parser.LhsExpressionContext expr : ctx.lhsExpression()) {
+ visitLhsExpression(expr);
+ }
+ e.depth--;
+ }
+
+ void visitLhsExpression(DRL10Parser.LhsExpressionContext ctx) {
+ e.emitHiddenTokensBefore(ctx);
+ if (ctx instanceof DRL10Parser.LhsExpressionEnclosedContext enclosed) {
+ emitGroup(consumePatternPrefix(), enclosed.lhsExpression());
+ } else if (!prefixKeyword(ctx).isEmpty()) {
+ e.emit(e.indent() + consumePatternPrefix() + prefixKeyword(ctx));
+ e.newline();
+ e.depth++;
+ visitOperands(ctx);
+ e.depth--;
+ } else if (ctx instanceof DRL10Parser.LhsOrContext orCtx) {
+ List<DRL10Parser.LhsExpressionContext> exprs = orCtx.lhsExpression();
+ for (int i = 0; i < exprs.size(); i++) {
+ if (i > 0) {
+ e.emit(e.indent() + "or");
+ e.newline();
+ }
+ visitLhsExpression(exprs.get(i));
+ }
+ } else if (ctx instanceof DRL10Parser.LhsAndContext andCtx) {
+ List<DRL10Parser.LhsExpressionContext> exprs = andCtx.lhsExpression();
+ for (int i = 0; i < exprs.size(); i++) {
+ if (i > 0) {
+ nextPatternPrefix = "and ";
+ }
+ visitLhsExpression(exprs.get(i));
+ }
+ } else if (ctx instanceof DRL10Parser.LhsUnarySingleContext unary) {
+ visitLhsUnary(unary.lhsUnary());
+ }
+ }
+
+ /**
+ * {@code or} or {@code and} when the element is written in prefix form,
+ * {@code (or A B)}, which is kept rather than rewritten as {@code A or B};
+ * empty otherwise.
+ */
+ private static String prefixKeyword(DRL10Parser.LhsExpressionContext ctx) {
+ int type = ctx.getStart().getType();
+ boolean prefix = (ctx instanceof DRL10Parser.LhsOrContext && type == DRL10Lexer.DRL_OR)
+ || (ctx instanceof DRL10Parser.LhsAndContext && type == DRL10Lexer.DRL_AND);
+ return prefix ? ctx.getStart().getText() : "";
+ }
+
+ private void visitOperands(DRL10Parser.LhsExpressionContext ctx) {
+ List<DRL10Parser.LhsExpressionContext> operands = ctx instanceof DRL10Parser.LhsOrContext orCtx
+ ? orCtx.lhsExpression()
+ : ((DRL10Parser.LhsAndContext) ctx).lhsExpression();
+ for (DRL10Parser.LhsExpressionContext operand : operands) {
+ visitLhsExpression(operand);
+ }
+ }
+
+ /**
+ * {@code head(} and {@code )} on their own lines with the element between
+ * them indented; a prefix-form element keeps its keyword on the opening
+ * line — {@code (or}, {@code not(and} — with its operands beneath.
+ */
+ private void emitGroup(String head, DRL10Parser.LhsExpressionContext inner) {
+ String keyword = prefixKeyword(inner);
+ e.emit(e.indent() + head + "(" + keyword);
+ e.newline();
+ e.depth++;
+ if (keyword.isEmpty()) {
+ visitLhsExpression(inner);
+ } else {
+ visitOperands(inner);
+ }
+ e.depth--;
+ e.emit(e.indent() + ")");
+ e.newline();
+ }
+
+ private void visitLhsUnary(DRL10Parser.LhsUnaryContext ctx) {
+ e.emitHiddenTokensBefore(ctx);
+ if (ctx.lhsPatternBind() != null) {
+ visitPatternBind(ctx.lhsPatternBind());
+ } else {
+ visitConditionalElement(ctx);
+ }
+ emitTrailingInvocations(ctx);
+ if (ctx.SEMI() != null && isOopath(ctx.lhsPatternBind())) {
+ // Before Drools 10 two OOPath lines without a ";" between them were read
+ // as one path, so this one terminator is the author's to keep.
+ e.appendToLastLine(";");
+ e.newline();
+ }
+ }
+
+ private static boolean isOopath(DRL10Parser.LhsPatternBindContext bind) {
+ return bind != null && bind.lhsPattern().stream().anyMatch(p -> p.xpathPrimary() != null);
+ }
+
+ private void visitConditionalElement(DRL10Parser.LhsUnaryContext ctx) {
+ // Consume any pending prefix (e.g. "and "): visitPatternBind does this
+ // itself, the other handlers need it here.
+ String prefix = consumePatternPrefix();
+ if (ctx.lhsExists() != null) {
+ visitLhsExists(ctx.lhsExists(), prefix);
+ } else if (ctx.lhsNot() != null) {
+ visitLhsNot(ctx.lhsNot(), prefix);
+ } else if (ctx.lhsEval() != null) {
+ String inner = e.styledText(ctx.lhsEval().conditionalOrExpression());
+ e.emit(e.indent() + prefix
+ + (e.options.parenPadding() ? "eval( " + inner + " )" : "eval(" + inner + ")"));
+ e.newline();
+ } else if (ctx.lhsForall() != null) {
+ visitLhsForall(ctx.lhsForall());
+ } else if (ctx.lhsAccumulate() != null) {
+ visitLhsAccumulate(ctx.lhsAccumulate());
+ } else if (ctx.lhsGroupBy() != null) {
+ visitLhsGroupBy(ctx.lhsGroupBy());
Review Comment:
The pending `and ` prefix is consumed before dispatch, but these three branches discard it because their helpers emit `forall(`/`accumulate(`/`groupby(` directly. Consequently, a valid condition such as `A() and accumulate(...)` loses the explicit connective and is then rejected by the content-preservation gate instead of being formatted. Pass the prefix into each helper and prepend it to its opening line.
##########
packages/drools-lsp/drools-formatter/src/main/java/org/drools/formatter/RhsFormatter.java:
##########
@@ -0,0 +1,517 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.drools.formatter;
+
+import java.util.ArrayList;
+import java.util.List;
+
+import org.antlr.v4.runtime.Token;
+import org.drools.drl.parser.antlr4.DRL10Parser;
+
+/**
+ * Formats consequences (then blocks): token-level, since the parser keeps them opaque.
+ *
+ * <p>The DRL parser treats {@code then} blocks as opaque text with individual
+ * tokens on a special RHS channel, so formatting works at the token level
+ * rather than the CST level:
+ * <ul>
+ * <li>{@code emitConsequenceBody} collects visible RHS tokens into
+ * per-statement lists, splitting only at newlines where paren depth is 0
+ * (so continuation lines within expanded calls stay together).</li>
+ * <li>{@code emitRhsStatement} tries the compact (single-line) form first.
+ * If it exceeds {@code options.lineLength()}, it delegates to
+ * {@code rhsEmitExpanded}.</li>
+ * <li>{@code rhsEmitExpanded} uses {@code rhsMarkExpanded} to decide which
+ * {@code (} groups need expansion: any group whose compact rendering
+ * (including trailing {@code );}) would exceed the line limit is marked.
+ * Marked groups get one argument per line with the closing {@code )}
+ * on its own line at the same indent as the opening.</li>
+ * </ul>
+ */
+final class RhsFormatter {
+ private final Emitter e;
+
+ // Extra depth added when a case/default label is emitted without an inline
+ // consequence (consequence follows on subsequent lines). Reset to 0 when
+ // the next case/default label or closing '}' is encountered.
+ private int caseLabelDepthBump = 0;
+
+ RhsFormatter(Emitter e) {
+ this.e = e;
+ }
+
+ void visitRhs(DRL10Parser.RhsContext ctx) {
+ e.emitHiddenTokensBefore(ctx);
+ e.emit(e.indent() + "then");
+ e.newline();
+ e.depth++;
+
+ emitConsequenceBody(ctx.consequenceBody());
+
+ for (DRL10Parser.NamedConsequenceContext nc : ctx.namedConsequence()) {
+ e.depth--;
+ // named consequence header: "then[name]"
+ String ncText = nc.RHS_NAMED_CONSEQUENCE_THEN().getText();
+ e.emit(e.indent() + ncText);
+ e.newline();
+ e.depth++;
+ emitConsequenceBody(nc.consequenceBody());
+ }
+
+ e.depth--;
+ }
+
+ private void emitConsequenceBody(DRL10Parser.ConsequenceBodyContext ctx) {
+ if (ctx == null || ctx.getChildCount() == 0) return;
+
+ int startIdx = ctx.getStart().getTokenIndex();
+ int stopIdx = ctx.getStop().getTokenIndex();
+
+ // Collect visible RHS tokens into statements. A statement boundary is
+ // a newline where paren depth is 0 (balanced). Newlines inside parens
+ // are continuation lines and belong to the same statement.
+ List<String> stmtTokens = new ArrayList<>();
+ boolean pendingBlankLine = false;
+ int parenDepth = 0;
+ int lastTokenLine = -1;
+
+ for (int i = startIdx; i <= stopIdx; i++) {
+ Token t = e.tokens.get(i);
+ String text = t.getText();
+
+ if (t.getChannel() == Token.HIDDEN_CHANNEL) {
+ if (text.contains("\n") && parenDepth == 0) {
+ if (!stmtTokens.isEmpty()) {
+ emitRhsStatementInBlock(stmtTokens);
+ stmtTokens.clear();
+ }
+ long nlCount = text.chars().filter(c -> c == '\n').count();
+ if (nlCount > 1) pendingBlankLine = true;
+ }
+ // Newlines inside parens are ignored (tokens collected into same statement)
+ continue;
+ }
+
+ if (Emitter.isComment(t)) {
+ // A comment sharing its source line with the previous token is a
+ // trailing comment (e.g. "insert( new Foo() ); // audit note") —
+ // flush the statement it trails, then reattach it to that line
+ // instead of dropping it onto a fresh one.
+ boolean trailsPreviousToken = t.getLine() == lastTokenLine && parenDepth == 0;
+ if (!stmtTokens.isEmpty() && parenDepth == 0) {
+ emitRhsStatementInBlock(stmtTokens);
+ stmtTokens.clear();
+ }
+ if (trailsPreviousToken) {
+ e.appendToLastLine(" " + text.trim());
+ e.newline();
+ } else {
+ if (pendingBlankLine) { e.blankLine(); pendingBlankLine = false; }
+ e.emit(e.indent() + text.trim());
+ e.newline();
+ }
+ continue;
+ }
+
+ // Track paren depth for statement boundary detection.
+ // Only count () — curly braces are block delimiters (switch, if, for)
+ // and should not suppress newline-based statement splitting.
+ if (text.equals("(")) parenDepth++;
+ else if (text.equals(")")) parenDepth = Math.max(0, parenDepth - 1);
+
+ // case/default keywords at top level act as statement boundaries so
+ // that multiple cases concatenated on one line are split correctly
+ // (e.g. badly-formatted input: "break; case 2: ...").
+ if (isCaseLabelStart(text) && parenDepth == 0 && !stmtTokens.isEmpty()) {
+ emitRhsStatementInBlock(stmtTokens);
+ stmtTokens.clear();
+ }
+
+ if (pendingBlankLine) { e.blankLine(); pendingBlankLine = false; }
+ stmtTokens.add(text);
+ lastTokenLine = t.getLine();
+ }
+ if (!stmtTokens.isEmpty()) {
+ emitRhsStatementInBlock(stmtTokens);
+ }
+
+ e.lastEmittedTokenIndex = Math.max(e.lastEmittedTokenIndex, stopIdx);
+ }
+
+ /**
+ * Wrapper around {@link #emitRhsStatement} that manages brace-depth and
+ * case/default label splitting so that switch blocks are formatted as:
+ * <pre>
+ * switch( x )
+ * {
+ * case 1:
+ * value = ...; break;
+ * default:
+ * value = ...;
+ * }
+ * </pre>
+ * Rules:
+ * <ul>
+ * <li>Statement starting with {@code }} → decrement depth before emit.</li>
+ * <li>Statement ending with {@code {} → increment depth after emit.</li>
+ * <li>Statement starting with {@code case} or {@code default} → split at
+ * the first top-level {@code :}, emit label at current depth, bump
+ * depth by 1, emit consequence, restore depth.</li>
+ * </ul>
+ */
+ private void emitRhsStatementInBlock(List<String> toks) {
+ if (toks.isEmpty()) return;
+
+ String first = toks.get(0);
+ String last = toks.get(toks.size() - 1);
+
+ // Closing brace: undo any case-consequence depth bump, then decrease brace indent.
+ if (first.equals("}")) {
+ e.depth -= caseLabelDepthBump;
+ caseLabelDepthBump = 0;
+ e.depth--;
Review Comment:
This unwinds the case-label indentation for every statement beginning with `}`, including a nested `if`, loop, or block inside the current case. After that inner block closes, the remaining statements in the same case are emitted one level too shallow; nested switches similarly lose the outer case state. Track case indentation by the corresponding switch/brace depth and unwind it only at the next label or the matching switch close.
--
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: commits-unsubscribe@kie.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@kie.apache.org
For additional commands, e-mail: commits-help@kie.apache.org
Re: [PR] kie-tools#3709: [drl-vscode-extension] add a configurable DRL formatter [incubator-kie-tools]
Posted by "drccrd (via GitHub)" <gi...@apache.org>.
drccrd commented on PR #4005:
URL: https://github.com/apache/incubator-kie-tools/pull/4005#issuecomment-5648023877
@tkobayas shall I send an email to the dev list for this PR as a heads-up? I think some wider feedback on format expectations would be good - otherwise if this is included in the 10.3.0 release user feedback can inform future changes.
--
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: commits-unsubscribe@kie.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@kie.apache.org
For additional commands, e-mail: commits-help@kie.apache.org
Re: [PR] kie-tools#3709: [drl-vscode-extension] add a configurable DRL formatter [incubator-kie-tools]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #4005:
URL: https://github.com/apache/incubator-kie-tools/pull/4005#discussion_r4003112534
##########
packages/drools-lsp/drools-formatter/src/main/java/org/drools/formatter/Emitter.java:
##########
@@ -0,0 +1,573 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.drools.formatter;
+
+import java.util.ArrayList;
+import java.util.List;
+
+import org.antlr.v4.runtime.CommonTokenStream;
+import org.antlr.v4.runtime.ParserRuleContext;
+import org.antlr.v4.runtime.Token;
+import org.antlr.v4.runtime.misc.Interval;
+import org.drools.drl.parser.antlr4.DRL10Lexer;
+import org.drools.drl.parser.antlr4.DRL10Parser;
+import org.drools.drl.parser.antlr4.DRL10ParserHelper;
+
+/**
+ * Output side of the formatter: the buffer, indentation depth, hidden-token and
+ * comment emission, {@code @formatter:off} regions, and the two text extractors
+ * ({@code styledText} normalizes spacing, {@code sourceSlice} is byte-faithful).
+ * Every area formatter writes through one instance of this.
+ *
+ * <p>Comments and blank lines from the hidden channel are emitted by
+ * {@code emitHiddenTokensBefore} / {@code emitTrailingHiddenTokens}, which
+ * walk the token stream between the last emitted index and the current node.
+ * Block comments are re-indented; line comments are preserved.
+ *
+ * <p>Emission is STYLE-NORMALIZING and COMMENT-PRESERVING, not byte-preserving:
+ * token content and comments always survive, while spacing and the sanctioned
+ * separator/terminator normalizations (statement {@code ;}, accumulate/groupby
+ * source separator {@code ,} → {@code ;}) come from the formatter, not the
+ * source.
+ * <ul>
+ * <li>{@code styledText} — the standard emission helper: visible tokens
+ * of a node (or token span) joined per the house spacing rules
+ * ({@code needsSpaceBetween}), block comments kept inline, hidden
+ * whitespace discarded.</li>
+ * <li>{@code sourceSlice} — exact original text of a node (or token span),
+ * including comments and inter-token whitespace. Reserved for the sites
+ * where verbatim IS the contract: frozen regions/{@code emitVerbatim},
+ * the unparsed-rule fallback, and the multi-line reflow guards that keep
+ * a line comment's newline.</li>
+ * <li>Both advance {@code lastEmittedTokenIndex} so hidden-token emission
+ * never double-prints what a span already contains.</li>
+ * <li>{@code singleLine} — collapses newline+indent runs to single spaces
+ * for one-line emission contexts.</li>
+ * </ul>
+ */
+final class Emitter {
+
+ final DRL10Parser parser;
+ final CommonTokenStream tokens;
+ final StringBuilder out = new StringBuilder();
+ final FormatterOptions options;
+ final TokenSpacing spacing;
+ int depth = 0;
+ int lastEmittedTokenIndex = -1;
+ int consecutiveNewlines = 0;
+
+ // Source-line ranges (1-based, inclusive) frozen by @formatter:off/on
+ // markers; top-level statements overlapping these are emitted verbatim.
+ // Populated once the token stream is filled, via computeSkipRegions().
+ List<int[]> skipRegions = List.of();
+
+ Emitter(String text, FormatterOptions options) {
+ parser = DRL10ParserHelper.createDrlParser(text);
+ tokens = (CommonTokenStream) parser.getTokenStream();
+ this.options = options;
+ this.spacing = new TokenSpacing(options);
+ }
+
+ String result() {
+ return out.toString();
+ }
+
+ // ── output helpers ────────────────────────────────────────────────────
+
+ String indent() {
+ return options.indentUnit().repeat(Math.max(0, depth));
+ }
+
+ String indent(int offset) {
+ return options.indentUnit().repeat(Math.max(0, depth + offset));
+ }
+
+ void emit(String text) {
+ out.append(text);
+ if (!text.isEmpty()) {
+ consecutiveNewlines = 0;
+ }
+ }
+
+ void newline() {
+ out.append('\n');
+ consecutiveNewlines++;
+ }
+
+ void blankLine() {
+ if (consecutiveNewlines < 2) {
+ newline();
+ }
+ }
+
+ /**
+ * Append text to the last line of output (before any trailing newline).
+ * Used to attach suffixes like "," to a pattern's closing ")".
+ */
+ void appendToLastLine(String suffix) {
+ // Strip trailing newline, append suffix. Caller must call newline() afterwards.
+ int len = out.length();
+ while (len > 0 && out.charAt(len - 1) == '\n') {
+ len--;
+ }
+ out.setLength(len);
+ out.append(suffix);
+ consecutiveNewlines = 0;
+ }
+
+ /**
+ * Emits a header line and its metadata items (annotations, attributes) per
+ * options.headerMetadata(), leaving depth one deeper than on entry — the
+ * body's level. Comments between items are emitted before the item they
+ * precede; under INLINE that places them on their own lines after the header.
+ */
+ void emitHeader(String header, List<? extends ParserRuleContext> items) {
+ FormatterOptions.HeaderMetadata mode = options.headerMetadata();
+ if (mode == FormatterOptions.HeaderMetadata.INLINE) {
+ StringBuilder line = new StringBuilder(header);
+ for (ParserRuleContext item : items) {
+ String text = styledText(item);
Review Comment:
`styledText(item)` advances `lastEmittedTokenIndex` while the inline header is only being buffered. By the time the second loop calls `emitHiddenTokensBefore`, the cursor is already past every metadata item, so comments between the header and an annotation/attribute (or between items) are silently dropped in `INLINE` mode. Collect the item text without advancing the emission cursor, or buffer and emit those hidden tokens before moving it.
##########
packages/drools-lsp/drools-lsp-server/src/main/java/org/drools/lsp/server/DroolsLspServer.java:
##########
@@ -441,9 +460,85 @@ public CompletableFuture<InitializeResult> initialize(InitializeParams params) {
});
}
+ textService.setFormatterOptions(formatterOptionsOf(params.getInitializationOptions()));
+
return CompletableFuture.supplyAsync(() -> initializeResult);
}
+ /**
+ * Pulls {@code drools.lsp.formatter} through {@code workspace/configuration} and
+ * registers for an empty configuration change, the pattern LSP 3.17 prescribes:
+ * "If the server still needs to react to configuration changes (since the server
+ * caches the result of {@code workspace/configuration} requests) the server should
+ * register for an empty configuration change using the following registration
+ * pattern" (LSP 3.17, workspace/configuration).
+ */
+ @Override
+ public void initialized(InitializedParams params) {
+ pullFormatterOptions();
+ LanguageClient target = client;
+ if (!clientSupportsConfigurationRegistration || target == null) {
+ return;
+ }
+ Registration registration = new Registration("drools.lsp.didChangeConfiguration",
+ "workspace/didChangeConfiguration");
+ try {
+ target.registerCapability(new RegistrationParams(List.of(registration)))
+ .exceptionally(e -> {
+ logger.log(Level.WARNING, "Client refused to register for configuration "
+ + "changes — formatter settings will need a restart", e);
+ return null;
+ });
+ } catch (Exception e) {
+ logger.log(Level.WARNING, "Client does not implement client/registerCapability", e);
+ }
+ }
+
+ CompletableFuture<Void> pullFormatterOptions() {
+ LanguageClient target = client;
+ if (!clientProvidesConfiguration || target == null) {
+ return CompletableFuture.completedFuture(null);
+ }
+ ConfigurationItem item = new ConfigurationItem();
+ item.setSection("drools.lsp.formatter");
+ try {
+ return target.configuration(new ConfigurationParams(List.of(item)))
+ .thenAccept(this::applyPulledFormatterOptions)
Review Comment:
Configuration pulls can be outstanding concurrently: startup and successive `didChangeConfiguration` notifications each call this method without chaining. JSON-RPC responses may complete out of order, allowing an older settings response to overwrite the newest live options. Sequence the pulls or attach a monotonically increasing generation and apply only the latest response.
##########
packages/drools-lsp/drools-formatter/src/main/java/org/drools/formatter/LhsFormatter.java:
##########
@@ -0,0 +1,671 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.drools.formatter;
+
+import java.util.List;
+
+import org.antlr.v4.runtime.Token;
+import org.drools.drl.parser.antlr4.DRL10Lexer;
+import org.drools.drl.parser.antlr4.DRL10Parser;
+
+/**
+ * Formats a rule's conditions (the when block) and a query's body.
+ *
+ * <p>This class walks the CST with typed {@code visit*} methods:
+ * <ul>
+ * <li>{@code visitLhs} → {@code visitLhsExpression} dispatches on
+ * expression type (and/or/unary/enclosed).</li>
+ * <li>{@code visitLhsUnary} handles patterns, not/exists, accumulate,
+ * forall, eval, and groupby.</li>
+ * <li>{@code visitPatternBind} renders a pattern on one line if it fits
+ * within {@code options.lineLength()} and its constraint parens hold no line
+ * comment (a {@code //} comment would swallow the rest of a one-line
+ * rendering); otherwise delegates to {@code emitReflowedPattern} which
+ * puts each constraint on its own line — trailing comments attached —
+ * with the closing {@code )} aligned to the opening.</li>
+ * <li>Parentheses are padded — {@code Person( age > 18 )} — whatever the
+ * source did, an empty pair staying {@code ()}. The rule is enforced in
+ * three places because parens reach the output three ways: token-by-token
+ * emission ({@code needsSpaceBetween}), verbatim RHS code
+ * ({@code rhsNeedsSpace}), and string-built signatures
+ * ({@code parenthesized}).</li>
+ * <li>{@code visitAndDef} emits explicit {@code and} keywords between
+ * patterns in accumulate/groupby source via a {@code nextPatternPrefix}
+ * mechanism.</li>
+ * </ul>
+ */
+final class LhsFormatter {
+ private final Emitter e;
+
+ // Prefix to prepend to the next pattern (e.g. "and " between patterns)
+ private String nextPatternPrefix = "";
+
+ LhsFormatter(Emitter e) {
+ this.e = e;
+ }
+
+ void visitLhs(DRL10Parser.LhsContext ctx) {
+ e.emitHiddenTokensBefore(ctx);
+ e.emit(e.indent() + "when");
+ e.newline();
+ e.depth++;
+ for (DRL10Parser.LhsExpressionContext expr : ctx.lhsExpression()) {
+ visitLhsExpression(expr);
+ }
+ e.depth--;
+ }
+
+ void visitLhsExpression(DRL10Parser.LhsExpressionContext ctx) {
+ e.emitHiddenTokensBefore(ctx);
+ if (ctx instanceof DRL10Parser.LhsExpressionEnclosedContext enclosed) {
+ e.emit(e.indent() + consumePatternPrefix() + "(");
+ e.newline();
+ e.depth++;
+ visitLhsExpression(enclosed.lhsExpression());
+ e.depth--;
+ e.emit(e.indent() + ")");
+ e.newline();
+ } else if (ctx instanceof DRL10Parser.LhsOrContext orCtx) {
+ List<DRL10Parser.LhsExpressionContext> exprs = orCtx.lhsExpression();
+ for (int i = 0; i < exprs.size(); i++) {
+ if (i > 0) {
+ e.emit(e.indent() + "or");
+ e.newline();
+ }
+ visitLhsExpression(exprs.get(i));
+ }
+ } else if (ctx instanceof DRL10Parser.LhsAndContext andCtx) {
+ List<DRL10Parser.LhsExpressionContext> exprs = andCtx.lhsExpression();
+ for (int i = 0; i < exprs.size(); i++) {
+ if (i > 0) {
+ nextPatternPrefix = "and ";
+ }
+ visitLhsExpression(exprs.get(i));
+ }
+ } else if (ctx instanceof DRL10Parser.LhsUnarySingleContext unary) {
+ visitLhsUnary(unary.lhsUnary());
+ }
+ }
+
+ private void visitLhsUnary(DRL10Parser.LhsUnaryContext ctx) {
+ e.emitHiddenTokensBefore(ctx);
+ // Consume any pending prefix (e.g. "and ") for non-pattern nodes.
+ // visitPatternBind consumes it itself; other handlers need it here.
+ if (ctx.lhsPatternBind() != null) {
+ visitPatternBind(ctx.lhsPatternBind());
+ emitTrailingInvocations(ctx);
+ return;
+ }
+ String prefix = consumePatternPrefix();
+ if (ctx.lhsExists() != null) {
+ visitLhsExists(ctx.lhsExists(), prefix);
+ } else if (ctx.lhsNot() != null) {
+ visitLhsNot(ctx.lhsNot(), prefix);
+ } else if (ctx.lhsEval() != null) {
+ String inner = e.styledText(ctx.lhsEval().conditionalOrExpression());
+ e.emit(e.indent() + prefix
+ + (e.options.parenPadding() ? "eval( " + inner + " )" : "eval(" + inner + ")"));
+ e.newline();
+ } else if (ctx.lhsForall() != null) {
+ visitLhsForall(ctx.lhsForall());
+ } else if (ctx.lhsAccumulate() != null) {
+ visitLhsAccumulate(ctx.lhsAccumulate());
+ } else if (ctx.lhsGroupBy() != null) {
+ visitLhsGroupBy(ctx.lhsGroupBy());
+ } else if (ctx.lhsExpression() != null) {
+ // parenthesized lhsExpression
+ e.emit(e.indent() + prefix + "(");
+ e.newline();
+ e.depth++;
+ visitLhsExpression(ctx.lhsExpression());
+ e.depth--;
+ e.emit(e.indent() + ")");
+ e.newline();
+ } else if (ctx.conditionalBranch() != null) {
+ e.emit(e.indent() + prefix + e.styledText(ctx.conditionalBranch()));
+ e.newline();
+ }
+ // exists/not/eval and a parenthesised group can each carry a trailing
+ // invocation too; the pattern-bind path above appends its own.
+ emitTrailingInvocations(ctx);
+ }
+
+ /**
+ * The trailing consequence invocations of an {@code lhsUnary}, rendered for
+ * the end of the element's line.
+ *
+ * <p>The grammar hangs these off {@code lhsUnary} as siblings of the element
+ * they qualify — {@code lhsPatternBind consequenceInvocation*},
+ * {@code lhsExists namedConsequenceInvocation?} — so visiting only the
+ * element drops them. Dropping one is silent: {@code $p : Bar( age > 18 ) if
+ * ( $p.age > 65 ) do[senior]} loses its guard while the {@code then[senior]}
+ * block survives, leaving a labelled consequence that can never fire, and the
+ * output still re-parses, so the output-reparse net cannot see it.
+ */
+ private String trailingInvocations(DRL10Parser.LhsUnaryContext ctx) {
+ StringBuilder sb = new StringBuilder();
+ for (DRL10Parser.ConsequenceInvocationContext invocation : ctx.consequenceInvocation()) {
+ sb.append(' ').append(e.styledText(invocation));
+ }
+ if (ctx.namedConsequenceInvocation() != null) {
+ sb.append(' ').append(e.styledText(ctx.namedConsequenceInvocation()));
+ }
+ return sb.toString();
+ }
+
+ /**
+ * Attach an {@code lhsUnary}'s trailing invocations to the element just
+ * emitted. The element may have taken several lines (reflowed, or forced
+ * multi-line by a comment), so this appends to the emitted text rather than
+ * to a current-line buffer.
+ */
+ private void emitTrailingInvocations(DRL10Parser.LhsUnaryContext ctx) {
+ String trailing = trailingInvocations(ctx);
+ if (trailing.isEmpty()) {
+ return;
+ }
+ e.appendToLastLine(trailing);
+ e.newline();
+ }
+
+ private void visitLhsExists(DRL10Parser.LhsExistsContext ctx, String prefix) {
+ if (ctx.lhsPatternBind() != null) {
+ if (emitMultiLineIfCommented(ctx.lhsPatternBind(), prefix + "exists ")) {
+ return;
+ }
+ String patternText = formatPatternBind(ctx.lhsPatternBind());
+ String line = e.indent() + prefix + "exists " + patternText;
+ if (line.length() <= e.options.lineLength()) {
+ e.emit(line);
+ e.newline();
+ } else {
+ emitReflowedPattern(ctx.lhsPatternBind(), prefix + "exists ");
+ }
+ } else if (ctx.lhsExpression() != null) {
+ e.emit(e.indent() + prefix + "exists(");
+ e.newline();
+ e.depth++;
+ visitLhsExpression(ctx.lhsExpression());
+ e.depth--;
+ e.emit(e.indent() + ")");
+ e.newline();
+ }
+ }
+
+ private void visitLhsNot(DRL10Parser.LhsNotContext ctx, String prefix) {
+ if (ctx.lhsPatternBind() != null) {
+ if (emitMultiLineIfCommented(ctx.lhsPatternBind(), prefix + "not ")) {
+ return;
+ }
+ String patternText = formatPatternBind(ctx.lhsPatternBind());
+ String line = e.indent() + prefix + "not " + patternText;
+ if (line.length() <= e.options.lineLength()) {
+ e.emit(line);
+ e.newline();
+ } else {
+ emitReflowedPattern(ctx.lhsPatternBind(), prefix + "not ");
+ }
+ } else if (ctx.lhsExpression() != null) {
+ e.emit(e.indent() + prefix + "not(");
+ e.newline();
+ e.depth++;
+ visitLhsExpression(ctx.lhsExpression());
+ e.depth--;
+ e.emit(e.indent() + ")");
+ e.newline();
+ }
+ }
+
+ private void visitLhsForall(DRL10Parser.LhsForallContext ctx) {
+ e.emit(e.indent() + "forall(");
+ e.newline();
+ e.depth++;
+ for (DRL10Parser.LhsPatternBindContext pb : ctx.lhsPatternBind()) {
+ visitPatternBind(pb);
+ }
+ e.depth--;
+ e.emit(e.indent() + ")");
+ e.newline();
+ }
+
+ private void visitLhsAccumulate(DRL10Parser.LhsAccumulateContext ctx) {
+ String keyword = ctx.DRL_ACC() != null ? "acc" : "accumulate";
+ e.emit(e.indent() + keyword + "(");
+ e.newline();
+ e.depth++;
+ // The grammar accepts "," or ";" after the source pattern, but the DRL
+ // language reference documents inline accumulate as
+ // accumulate( <source pattern>; <functions> [;<constraints>] )
+ // so a source "," is normalized to the documented ";" form.
+ visitAndDef(ctx.lhsAndDef());
+ e.appendToLastLine(e.options.normalizeTerminators() ? ";" : sourceSeparator(ctx.lhsAndDef()));
+ e.newline();
+ // accumulate functions
+ List<DRL10Parser.AccumulateFunctionContext> funcs = ctx.accumulateFunction();
+ for (int i = 0; i < funcs.size(); i++) {
+ e.emit(e.indent() + e.styledText(funcs.get(i)));
Review Comment:
Comments between the accumulate source separator and the first function, or between functions, are skipped here: `styledText` starts at the function's visible token and then advances the global cursor past the preceding hidden tokens without emitting them. The same applies before trailing constraints/`)`. Emit the intervening hidden tokens for each child before advancing the cursor so formatting cannot delete source comments.
This issue also appears on line 288 of the same file.
--
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: commits-unsubscribe@kie.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@kie.apache.org
For additional commands, e-mail: commits-help@kie.apache.org
Re: [PR] incubator-kie-tools#3709: [drl-vscode-extension] add a configurable DRL formatter [incubator-kie-tools]
Posted by "drccrd (via GitHub)" <gi...@apache.org>.
drccrd commented on PR #4005:
URL: https://github.com/apache/incubator-kie-tools/pull/4005#issuecomment-5752963767
@jomarko thank you very much for testing!
These are critical points you identified - indeed the majority of the testing during development was conducted on traditional syntax, so RuleUnit and OOPath syntax weren't correctly represented.
I have adjusted the logic in [3606543](https://github.com/apache/incubator-kie-tools/pull/4005/commits/3606543f9ef38220e5b1cfa16bb50cf319eb8e0f) and [b59e8fc](https://github.com/apache/incubator-kie-tools/pull/4005/commits/b59e8fc633f14daca9c76326c45f3ed299dc35b1) to keep OOpath expression chains without spaces, and add spaces between square brackets depending on the configurable 'parenPadding' to align with parentheses and braces.
I got claude to build an additional check for any lost tokens during formatting, and ran the formatter across all drl files in incubator-kie and incubator-kie-tools and found a few bugs, and also a few supported syntax patterns I wasn't aware of. These have been addressed in the subsequent commits and added to the readme.
--
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: commits-unsubscribe@kie.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@kie.apache.org
For additional commands, e-mail: commits-help@kie.apache.org
Re: [PR] kie-tools#3709: [drl-vscode-extension] add a configurable DRL formatter [incubator-kie-tools]
Posted by "drccrd (via GitHub)" <gi...@apache.org>.
drccrd commented on code in PR #4005:
URL: https://github.com/apache/incubator-kie-tools/pull/4005#discussion_r4004658620
##########
packages/drools-lsp/drools-lsp-server/src/main/java/org/drools/lsp/server/DroolsLspServer.java:
##########
@@ -441,9 +460,85 @@ public CompletableFuture<InitializeResult> initialize(InitializeParams params) {
});
}
+ textService.setFormatterOptions(formatterOptionsOf(params.getInitializationOptions()));
+
return CompletableFuture.supplyAsync(() -> initializeResult);
}
+ /**
+ * Pulls {@code drools.lsp.formatter} through {@code workspace/configuration} and
+ * registers for an empty configuration change, the pattern LSP 3.17 prescribes:
+ * "If the server still needs to react to configuration changes (since the server
+ * caches the result of {@code workspace/configuration} requests) the server should
+ * register for an empty configuration change using the following registration
+ * pattern" (LSP 3.17, workspace/configuration).
+ */
+ @Override
+ public void initialized(InitializedParams params) {
+ pullFormatterOptions();
+ LanguageClient target = client;
+ if (!clientSupportsConfigurationRegistration || target == null) {
+ return;
+ }
+ Registration registration = new Registration("drools.lsp.didChangeConfiguration",
+ "workspace/didChangeConfiguration");
+ try {
+ target.registerCapability(new RegistrationParams(List.of(registration)))
+ .exceptionally(e -> {
+ logger.log(Level.WARNING, "Client refused to register for configuration "
+ + "changes — formatter settings will need a restart", e);
+ return null;
+ });
+ } catch (Exception e) {
+ logger.log(Level.WARNING, "Client does not implement client/registerCapability", e);
+ }
+ }
+
+ CompletableFuture<Void> pullFormatterOptions() {
+ LanguageClient target = client;
+ if (!clientProvidesConfiguration || target == null) {
+ return CompletableFuture.completedFuture(null);
+ }
+ ConfigurationItem item = new ConfigurationItem();
+ item.setSection("drools.lsp.formatter");
+ try {
+ return target.configuration(new ConfigurationParams(List.of(item)))
+ .thenAccept(this::applyPulledFormatterOptions)
Review Comment:
Fixed in [cbf40c2](https://github.com/apache/incubator-kie-tools/pull/4005/commits/cbf40c2e2eb767d95739d5951df5dba4a52c5f0d)
--
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: commits-unsubscribe@kie.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@kie.apache.org
For additional commands, e-mail: commits-help@kie.apache.org
Re: [PR] incubator-kie-tools#3709: [drl-vscode-extension] add a configurable DRL formatter [incubator-kie-tools]
Posted by "drccrd (via GitHub)" <gi...@apache.org>.
drccrd commented on code in PR #4005:
URL: https://github.com/apache/incubator-kie-tools/pull/4005#discussion_r4062317753
##########
packages/drools-lsp/drools-formatter/src/main/java/org/drools/formatter/LhsFormatter.java:
##########
@@ -0,0 +1,723 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.drools.formatter;
+
+import java.util.List;
+
+import org.antlr.v4.runtime.Token;
+import org.drools.drl.parser.antlr4.DRL10Lexer;
+import org.drools.drl.parser.antlr4.DRL10Parser;
+
+/**
+ * Formats a rule's conditions (the when block) and a query's body.
+ *
+ * <p>This class walks the CST with typed {@code visit*} methods:
+ * <ul>
+ * <li>{@code visitLhs} → {@code visitLhsExpression} dispatches on
+ * expression type (and/or/unary/enclosed).</li>
+ * <li>{@code visitLhsUnary} handles patterns, not/exists, accumulate,
+ * forall, eval, and groupby.</li>
+ * <li>{@code visitPatternBind} renders a pattern on one line if it fits
+ * within {@code options.lineLength()} and its constraint parens hold no line
+ * comment (a {@code //} comment would swallow the rest of a one-line
+ * rendering); otherwise delegates to {@code emitReflowedPattern} which
+ * puts each constraint on its own line — trailing comments attached —
+ * with the closing {@code )} aligned to the opening.</li>
+ * <li>Parentheses are padded — {@code Person( age > 18 )} — whatever the
+ * source did, an empty pair staying {@code ()}. The rule is enforced in
+ * three places because parens reach the output three ways: token-by-token
+ * emission ({@code needsSpaceBetween}), verbatim RHS code
+ * ({@code rhsNeedsSpace}), and string-built signatures
+ * ({@code parenthesized}).</li>
+ * <li>{@code visitAndDef} emits explicit {@code and} keywords between
+ * patterns in accumulate/groupby source via a {@code nextPatternPrefix}
+ * mechanism.</li>
+ * </ul>
+ */
+final class LhsFormatter {
+ private final Emitter e;
+
+ // Prefix to prepend to the next pattern (e.g. "and " between patterns)
+ private String nextPatternPrefix = "";
+
+ LhsFormatter(Emitter e) {
+ this.e = e;
+ }
+
+ void visitLhs(DRL10Parser.LhsContext ctx) {
+ e.emitHiddenTokensBefore(ctx);
+ e.emit(e.indent() + "when");
+ e.newline();
+ e.depth++;
+ for (DRL10Parser.LhsExpressionContext expr : ctx.lhsExpression()) {
+ visitLhsExpression(expr);
+ }
+ e.depth--;
+ }
+
+ void visitLhsExpression(DRL10Parser.LhsExpressionContext ctx) {
+ e.emitHiddenTokensBefore(ctx);
+ if (ctx instanceof DRL10Parser.LhsExpressionEnclosedContext enclosed) {
+ emitGroup(consumePatternPrefix(), enclosed.lhsExpression());
+ } else if (!prefixKeyword(ctx).isEmpty()) {
+ e.emit(e.indent() + consumePatternPrefix() + prefixKeyword(ctx));
+ e.newline();
+ e.depth++;
+ visitOperands(ctx);
+ e.depth--;
+ } else if (ctx instanceof DRL10Parser.LhsOrContext orCtx) {
+ List<DRL10Parser.LhsExpressionContext> exprs = orCtx.lhsExpression();
+ for (int i = 0; i < exprs.size(); i++) {
+ if (i > 0) {
+ e.emit(e.indent() + "or");
+ e.newline();
+ }
+ visitLhsExpression(exprs.get(i));
+ }
+ } else if (ctx instanceof DRL10Parser.LhsAndContext andCtx) {
+ List<DRL10Parser.LhsExpressionContext> exprs = andCtx.lhsExpression();
+ for (int i = 0; i < exprs.size(); i++) {
+ if (i > 0) {
+ nextPatternPrefix = "and ";
+ }
+ visitLhsExpression(exprs.get(i));
+ }
+ } else if (ctx instanceof DRL10Parser.LhsUnarySingleContext unary) {
+ visitLhsUnary(unary.lhsUnary());
+ }
+ }
+
+ /**
+ * {@code or} or {@code and} when the element is written in prefix form,
+ * {@code (or A B)}, which is kept rather than rewritten as {@code A or B};
+ * empty otherwise.
+ */
+ private static String prefixKeyword(DRL10Parser.LhsExpressionContext ctx) {
+ int type = ctx.getStart().getType();
+ boolean prefix = (ctx instanceof DRL10Parser.LhsOrContext && type == DRL10Lexer.DRL_OR)
+ || (ctx instanceof DRL10Parser.LhsAndContext && type == DRL10Lexer.DRL_AND);
+ return prefix ? ctx.getStart().getText() : "";
+ }
+
+ private void visitOperands(DRL10Parser.LhsExpressionContext ctx) {
+ List<DRL10Parser.LhsExpressionContext> operands = ctx instanceof DRL10Parser.LhsOrContext orCtx
+ ? orCtx.lhsExpression()
+ : ((DRL10Parser.LhsAndContext) ctx).lhsExpression();
+ for (DRL10Parser.LhsExpressionContext operand : operands) {
+ visitLhsExpression(operand);
+ }
+ }
+
+ /**
+ * {@code head(} and {@code )} on their own lines with the element between
+ * them indented; a prefix-form element keeps its keyword on the opening
+ * line — {@code (or}, {@code not(and} — with its operands beneath.
+ */
+ private void emitGroup(String head, DRL10Parser.LhsExpressionContext inner) {
+ String keyword = prefixKeyword(inner);
+ e.emit(e.indent() + head + "(" + keyword);
+ e.newline();
+ e.depth++;
+ if (keyword.isEmpty()) {
+ visitLhsExpression(inner);
+ } else {
+ visitOperands(inner);
+ }
+ e.depth--;
+ e.emit(e.indent() + ")");
+ e.newline();
+ }
+
+ private void visitLhsUnary(DRL10Parser.LhsUnaryContext ctx) {
+ e.emitHiddenTokensBefore(ctx);
+ if (ctx.lhsPatternBind() != null) {
+ visitPatternBind(ctx.lhsPatternBind());
+ } else {
+ visitConditionalElement(ctx);
+ }
+ emitTrailingInvocations(ctx);
+ if (ctx.SEMI() != null && isOopath(ctx.lhsPatternBind())) {
+ // Before Drools 10 two OOPath lines without a ";" between them were read
+ // as one path, so this one terminator is the author's to keep.
+ e.appendToLastLine(";");
+ e.newline();
+ }
+ }
+
+ private static boolean isOopath(DRL10Parser.LhsPatternBindContext bind) {
+ return bind != null && bind.lhsPattern().stream().anyMatch(p -> p.xpathPrimary() != null);
+ }
+
+ private void visitConditionalElement(DRL10Parser.LhsUnaryContext ctx) {
+ // Consume any pending prefix (e.g. "and "): visitPatternBind does this
+ // itself, the other handlers need it here.
+ String prefix = consumePatternPrefix();
+ if (ctx.lhsExists() != null) {
+ visitLhsExists(ctx.lhsExists(), prefix);
+ } else if (ctx.lhsNot() != null) {
+ visitLhsNot(ctx.lhsNot(), prefix);
+ } else if (ctx.lhsEval() != null) {
+ String inner = e.styledText(ctx.lhsEval().conditionalOrExpression());
+ e.emit(e.indent() + prefix
+ + (e.options.parenPadding() ? "eval( " + inner + " )" : "eval(" + inner + ")"));
+ e.newline();
+ } else if (ctx.lhsForall() != null) {
+ visitLhsForall(ctx.lhsForall());
+ } else if (ctx.lhsAccumulate() != null) {
+ visitLhsAccumulate(ctx.lhsAccumulate());
+ } else if (ctx.lhsGroupBy() != null) {
+ visitLhsGroupBy(ctx.lhsGroupBy());
Review Comment:
should definitely have been major! fixed with [6c50f68](https://github.com/apache/incubator-kie-tools/pull/4005/commits/6c50f68390d8d5360c54d4620bb99c430c6f2ed8)
--
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: commits-unsubscribe@kie.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@kie.apache.org
For additional commands, e-mail: commits-help@kie.apache.org
Re: [PR] incubator-kie-tools#3709: [drl-vscode-extension] add a configurable DRL formatter [incubator-kie-tools]
Posted by "yesamer (via GitHub)" <gi...@apache.org>.
yesamer merged PR #4005:
URL: https://github.com/apache/incubator-kie-tools/pull/4005
--
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: commits-unsubscribe@kie.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@kie.apache.org
For additional commands, e-mail: commits-help@kie.apache.org
Re: [PR] kie-tools#3709: [drl-vscode-extension] add a configurable DRL formatter [incubator-kie-tools]
Posted by "drccrd (via GitHub)" <gi...@apache.org>.
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: commits-unsubscribe@kie.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@kie.apache.org
For additional commands, e-mail: commits-help@kie.apache.org
Re: [PR] kie-tools#3709: [drl-vscode-extension] add a configurable DRL formatter [incubator-kie-tools]
Posted by "drccrd (via GitHub)" <gi...@apache.org>.
drccrd commented on code in PR #4005:
URL: https://github.com/apache/incubator-kie-tools/pull/4005#discussion_r4004141097
##########
packages/drools-lsp/drools-formatter/src/main/java/org/drools/formatter/Emitter.java:
##########
@@ -0,0 +1,573 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.drools.formatter;
+
+import java.util.ArrayList;
+import java.util.List;
+
+import org.antlr.v4.runtime.CommonTokenStream;
+import org.antlr.v4.runtime.ParserRuleContext;
+import org.antlr.v4.runtime.Token;
+import org.antlr.v4.runtime.misc.Interval;
+import org.drools.drl.parser.antlr4.DRL10Lexer;
+import org.drools.drl.parser.antlr4.DRL10Parser;
+import org.drools.drl.parser.antlr4.DRL10ParserHelper;
+
+/**
+ * Output side of the formatter: the buffer, indentation depth, hidden-token and
+ * comment emission, {@code @formatter:off} regions, and the two text extractors
+ * ({@code styledText} normalizes spacing, {@code sourceSlice} is byte-faithful).
+ * Every area formatter writes through one instance of this.
+ *
+ * <p>Comments and blank lines from the hidden channel are emitted by
+ * {@code emitHiddenTokensBefore} / {@code emitTrailingHiddenTokens}, which
+ * walk the token stream between the last emitted index and the current node.
+ * Block comments are re-indented; line comments are preserved.
+ *
+ * <p>Emission is STYLE-NORMALIZING and COMMENT-PRESERVING, not byte-preserving:
+ * token content and comments always survive, while spacing and the sanctioned
+ * separator/terminator normalizations (statement {@code ;}, accumulate/groupby
+ * source separator {@code ,} → {@code ;}) come from the formatter, not the
+ * source.
+ * <ul>
+ * <li>{@code styledText} — the standard emission helper: visible tokens
+ * of a node (or token span) joined per the house spacing rules
+ * ({@code needsSpaceBetween}), block comments kept inline, hidden
+ * whitespace discarded.</li>
+ * <li>{@code sourceSlice} — exact original text of a node (or token span),
+ * including comments and inter-token whitespace. Reserved for the sites
+ * where verbatim IS the contract: frozen regions/{@code emitVerbatim},
+ * the unparsed-rule fallback, and the multi-line reflow guards that keep
+ * a line comment's newline.</li>
+ * <li>Both advance {@code lastEmittedTokenIndex} so hidden-token emission
+ * never double-prints what a span already contains.</li>
+ * <li>{@code singleLine} — collapses newline+indent runs to single spaces
+ * for one-line emission contexts.</li>
+ * </ul>
+ */
+final class Emitter {
+
+ final DRL10Parser parser;
+ final CommonTokenStream tokens;
+ final StringBuilder out = new StringBuilder();
+ final FormatterOptions options;
+ final TokenSpacing spacing;
+ int depth = 0;
+ int lastEmittedTokenIndex = -1;
+ int consecutiveNewlines = 0;
+
+ // Source-line ranges (1-based, inclusive) frozen by @formatter:off/on
+ // markers; top-level statements overlapping these are emitted verbatim.
+ // Populated once the token stream is filled, via computeSkipRegions().
+ List<int[]> skipRegions = List.of();
+
+ Emitter(String text, FormatterOptions options) {
+ parser = DRL10ParserHelper.createDrlParser(text);
+ tokens = (CommonTokenStream) parser.getTokenStream();
+ this.options = options;
+ this.spacing = new TokenSpacing(options);
+ }
+
+ String result() {
+ return out.toString();
+ }
+
+ // ── output helpers ────────────────────────────────────────────────────
+
+ String indent() {
+ return options.indentUnit().repeat(Math.max(0, depth));
+ }
+
+ String indent(int offset) {
+ return options.indentUnit().repeat(Math.max(0, depth + offset));
+ }
+
+ void emit(String text) {
+ out.append(text);
+ if (!text.isEmpty()) {
+ consecutiveNewlines = 0;
+ }
+ }
+
+ void newline() {
+ out.append('\n');
+ consecutiveNewlines++;
+ }
+
+ void blankLine() {
+ if (consecutiveNewlines < 2) {
+ newline();
+ }
+ }
+
+ /**
+ * Append text to the last line of output (before any trailing newline).
+ * Used to attach suffixes like "," to a pattern's closing ")".
+ */
+ void appendToLastLine(String suffix) {
+ // Strip trailing newline, append suffix. Caller must call newline() afterwards.
+ int len = out.length();
+ while (len > 0 && out.charAt(len - 1) == '\n') {
+ len--;
+ }
+ out.setLength(len);
+ out.append(suffix);
+ consecutiveNewlines = 0;
+ }
+
+ /**
+ * Emits a header line and its metadata items (annotations, attributes) per
+ * options.headerMetadata(), leaving depth one deeper than on entry — the
+ * body's level. Comments between items are emitted before the item they
+ * precede; under INLINE that places them on their own lines after the header.
+ */
+ void emitHeader(String header, List<? extends ParserRuleContext> items) {
+ FormatterOptions.HeaderMetadata mode = options.headerMetadata();
+ if (mode == FormatterOptions.HeaderMetadata.INLINE) {
+ StringBuilder line = new StringBuilder(header);
+ for (ParserRuleContext item : items) {
+ String text = styledText(item);
Review Comment:
Same as within an accumulate - I don't think inline comments between a rule name and metadata is something that should be supported.
--
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: commits-unsubscribe@kie.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@kie.apache.org
For additional commands, e-mail: commits-help@kie.apache.org
Re: [PR] kie-tools#3709: [drl-vscode-extension] add a configurable DRL formatter [incubator-kie-tools]
Posted by "tkobayas (via GitHub)" <gi...@apache.org>.
tkobayas commented on PR #4005:
URL: https://github.com/apache/incubator-kie-tools/pull/4005#issuecomment-5661346058
> @tkobayas shall I send an email to the dev list for this PR as a heads-up? I think some wider feedback on format expectations would be good - otherwise if this is included in the 10.3.0 release user feedback can inform future changes.
Yeah, a heads-up on the dev list would be great.
--
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: commits-unsubscribe@kie.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@kie.apache.org
For additional commands, e-mail: commits-help@kie.apache.org