Диагностика бессмысленного тернарного оператора #3629
Conversation
…a000cc13549a74b142a648823 author [email protected]
WalkthroughAdds a new UselessTernaryOperator diagnostic with quick-fix support, extends DiagnosticStorage to carry supplemental diagnostic data, updates configuration/schema and localization, adds documentation and unit tests. Changes
Sequence Diagram(s)sequenceDiagram
participant Parser as BSL Parser
participant Diagnoser as UselessTernaryOperatorDiagnostic
participant Storage as DiagnosticStorage
participant Client as LSP Client
participant DocCtx as DocumentContext
Parser->>Diagnoser: visitTernaryOperator(node)
Diagnoser->>Diagnoser: analyze condition and branches
alt pattern detected with quick-fix data
Diagnoser->>Storage: addDiagnostic(range, DiagnosticAdditionalData, message)
else simple diagnostic
Diagnoser->>Storage: addDiagnostic(range, message)
end
Storage->>Client: publishDiagnostic(Diagnostic)
Client->>Diagnoser: request quick fixes (CodeActionParams)
Diagnoser->>DocCtx: compute TextEdits using DiagnosticAdditionalData
Diagnoser->>Client: return CodeAction(s) with edits
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
✅ Files skipped from review due to trivial changes (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (8)
docs/diagnostics/UselessTernaryOperator.md (1)
4-26: Добавить примеры исправленного кода / упоминание квикфиксаОписание и примеры «плохого» кода читаются нормально, но сейчас нет ни одного явного примера исправления. По гайдам для
docs/*/diagnostics/**/*.mdлучше добавить хотя бы короткий блок с «Исправлениями» (например, замена?(Б = 1, Истина, Ложь)на простоБ = 1и?(Б = 0, Ложь, Истина)наНЕ (Б = 0)), и/или явно упомянуть, что часть случаев может быть автоматически исправлена квикфиксом.docs/en/diagnostics/UselessTernaryOperator.md (1)
4-27: Slight wording polish and add fix examplesTwo minor suggestions:
- The sentence “indicates poor code thoughtfulness” is a bit awkward; something like “indicates poor code design or readability” would read more naturally.
- As in the Russian doc, it would be useful to add a short “Fixes” section showing the equivalent simplified code (e.g.
A = B = 1;andA = NOT (B = 0);) to match the guideline of providing both problematic and fixed examples.- The placement of Boolean constants "True" or "False" in the ternary operator indicates poor code thoughtfulness. + The placement of Boolean constants "True" or "False" in the ternary operator usually indicates poor code design or readability.src/main/resources/com/github/_1c_syntax/bsl/languageserver/configuration/parameters-schema.json (1)
2111-2120: Schema definition is fine; just confirm default behaviorНовая
definitions.UselessTernaryOperatorполностью повторяет шаблон соседних диагностик (описание,type: ["boolean","object"],$id,title). Единственный момент на проверку —default: true: если политика проекта для новых диагностик не всегда подразумевает включение “по умолчанию”, стоит явно подтвердить, что это ожидаемое поведение.src/main/resources/com/github/_1c_syntax/bsl/languageserver/diagnostics/UselessTernaryOperatorDiagnostic_en.properties (1)
1-4: English messages OK; optional wording tweakСтроки полностью согласованы с реализацией. Если хочется чуть проще и точнее по смыслу, можно сделать quick-fix сообщение в единственном числе, т.к. обычно пользователь применяет его к одному тернарнику:
-quickFixMessage=Fix some useless ternary operators +quickFixMessage=Fix useless ternary operatorНо это скорее вкусовщина, текущий вариант тоже приемлем.
src/test/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/UselessTernaryOperatorDiagnosticTest.java (1)
39-84: Tests cover the important paths; only minor robustness/naming nitsТесты хорошо фиксируют поведение: количество диагностик, их диапазоны и точный текст квикфиксов для обоих случаев (прямая и инвертированная форма).
Пара необязательных улучшений на будущее:
- Сейчас
testQuickFixполагается на порядок срабатывания диагностик (diagnostics.get(0/1)). Если когда‑нибудь изменится обход дерева/порядок добавления, тест начнёт падать, хотя поведение останется корректным. Можно было бы искать нужные диагностики поRangeили тексту/данным.- Имена
reversDiagnostic/reversQuickFixлучше поправить наreverse...для читаемости.Оба пункта не блокирующие.
src/main/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/UselessTernaryOperatorDiagnostic.java (1)
42-128: Diagnostic/quick‑fix logic is solid; consider makinggetBooleanTokenmore grammar‑robustВ целом реализация выглядит аккуратно и хорошо согласована с документацией/тестами:
- В
visitTernaryOperatorкорректно обрабатываются все интересующие случаи:
- константа в условии → “бесполезный” тернарник без автокоррекции;
- ветки
Истина/True+Ложь/Falseи наоборот → диагностика + допданные для квикфикса;- любые другие комбинации с булевыми константами в ветках → “подозрительный” оператор без квикфикса.
- Использование
DiagnosticStorage.DiagnosticAdditionalDataи локализованного шаблонаquickFixAdaptedTextделает квикфикс чистым и расширяемым.- Sentinel
SKIPPED_RULE_INDEX = 0безопасен, т.к. токены ANTLR начинаются с 1.Единственное место, где есть потенциальная хрупкость на будущее —
getBooleanToken:var tmpCtx = Optional.of(expCtx) .filter(ctx -> ctx.children.size() == 1) .map(ctx -> ctx.member(0)) .map(ctx -> ctx.getChild(0)) .filter(BSLParser.ConstValueContext.class::isInstance) .map(BSLParser.ConstValueContext.class::cast);Этот код опирается на текущую структуру грамматики (
expressionс ровно одним потомком‑member и тем фактом, что первый child —ConstValueContext). Если грамматикаexpressionкогда‑нибудь поменяется (например, появится другой wrapper‑контекст), тут можно словить NPE/IOOB либо перестать распознавать булевы константы.Не критично на сейчас, но как небольшой буст к надёжности можно было бы:
- либо использовать специализированный подтип контекста (если в грамматике есть отдельная альтернатива для константных выражений),
- либо добавить дополнительные проверки (
ctx.member().size() == 1,instanceofдляctx.member(0).getChild(0)и т.п.).В остальном реализация выглядит корректной и достаточно простой для сопровождения.
src/main/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/DiagnosticStorage.java (2)
247-265: Consider reducing duplication by delegating from the existing overload.The method at lines 228-245 could delegate to this one with
nullfor data, reducing code duplication.public void addDiagnostic( Range range, String diagnosticMessage, @Nullable List<DiagnosticRelatedInformation> relatedInformation ) { - - if (Ranges.isEmpty(range)) { - return; - } - - diagnosticList.add(createDiagnostic( - diagnostic, + addDiagnostic( range, null, diagnosticMessage, relatedInformation - )); + ); }
286-294: Consider adding null-safety for the input string.The factory method doesn't validate the input. If
nullis passed,DiagnosticAdditionalDatawill hold a null value, which may cause issues when the data is consumed by quickfixes.public static DiagnosticAdditionalData createAdditionalData(String string) { + if (string == null) { + throw new IllegalArgumentException("Additional data string must not be null"); + } return new DiagnosticAdditionalData(string); }Alternatively, annotate the parameter with
@NonNullor add a@Nullableannotation to the record field if null is intentionally allowed.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
src/test/resources/diagnostics/UselessTernaryOperatorDiagnostic.bslis excluded by!src/test/resources/**
📒 Files selected for processing (9)
docs/diagnostics/UselessTernaryOperator.md(1 hunks)docs/en/diagnostics/UselessTernaryOperator.md(1 hunks)src/main/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/DiagnosticStorage.java(7 hunks)src/main/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/UselessTernaryOperatorDiagnostic.java(1 hunks)src/main/resources/com/github/_1c_syntax/bsl/languageserver/configuration/parameters-schema.json(1 hunks)src/main/resources/com/github/_1c_syntax/bsl/languageserver/configuration/schema.json(3 hunks)src/main/resources/com/github/_1c_syntax/bsl/languageserver/diagnostics/UselessTernaryOperatorDiagnostic_en.properties(1 hunks)src/main/resources/com/github/_1c_syntax/bsl/languageserver/diagnostics/UselessTernaryOperatorDiagnostic_ru.properties(1 hunks)src/test/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/UselessTernaryOperatorDiagnosticTest.java(1 hunks)
🧰 Additional context used
📓 Path-based instructions (7)
**/*.md
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Keep documentation up to date with code changes
Files:
docs/diagnostics/UselessTernaryOperator.mddocs/en/diagnostics/UselessTernaryOperator.md
**/diagnostics/*.properties
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Resource bundles for diagnostics should have both Russian and English versions
Files:
src/main/resources/com/github/_1c_syntax/bsl/languageserver/diagnostics/UselessTernaryOperatorDiagnostic_en.propertiessrc/main/resources/com/github/_1c_syntax/bsl/languageserver/diagnostics/UselessTernaryOperatorDiagnostic_ru.properties
**/*.java
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.java: Follow the Style Guide provided in docs/en/contributing/StyleGuide.md
Use Lombok annotations to reduce boilerplate code and enable annotation processing in your IDE
Optimize imports before committing but do NOT optimize imports across the entire project unless specifically working on that task
Follow Java naming conventions with meaningful, descriptive names; keep class and method names concise but clear
Write JavaDoc for public APIs and include comments for complex logic
Use Target Java 17 as the language version
Files:
src/main/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/UselessTernaryOperatorDiagnostic.javasrc/test/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/UselessTernaryOperatorDiagnosticTest.javasrc/main/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/DiagnosticStorage.java
**/diagnostics/*.java
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Each diagnostic should have a Java implementation class, resource bundle for localized messages, unit tests, and documentation
Files:
src/main/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/UselessTernaryOperatorDiagnostic.javasrc/test/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/UselessTernaryOperatorDiagnosticTest.javasrc/main/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/DiagnosticStorage.java
**/diagnostics/*Test.java
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Write comprehensive unit tests for each diagnostic including test cases for edge cases, following existing test patterns
Files:
src/test/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/UselessTernaryOperatorDiagnosticTest.java
**/test/java/**/*.java
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use appropriate test frameworks (JUnit, AssertJ, Mockito) for testing
Files:
src/test/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/UselessTernaryOperatorDiagnosticTest.java
docs/*/diagnostics/**/*.md
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Update diagnostic documentation in both Russian and English with examples of problematic code and fixes
Files:
docs/en/diagnostics/UselessTernaryOperator.md
🧠 Learnings (9)
📓 Common learnings
Learnt from: CR
Repo: 1c-syntax/bsl-language-server PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-11-27T07:17:33.726Z
Learning: Applies to docs/*/diagnostics/**/*.md : Update diagnostic documentation in both Russian and English with examples of problematic code and fixes
Learnt from: CR
Repo: 1c-syntax/bsl-language-server PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-11-27T07:17:33.726Z
Learning: Applies to **/diagnostics/*Test.java : Write comprehensive unit tests for each diagnostic including test cases for edge cases, following existing test patterns
Learnt from: nixel2007
Repo: 1c-syntax/bsl-language-server PR: 3308
File: src/test/resources/diagnostics/DoubleNegativesDiagnostic.bsl:20-21
Timestamp: 2024-07-04T19:35:26.747Z
Learning: The file `DoubleNegativesDiagnostic.bsl` is a test resource intended to demonstrate the functionality of the DoubleNegatives diagnostic.
Learnt from: CR
Repo: 1c-syntax/bsl-language-server PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-11-27T07:17:33.726Z
Learning: Applies to **/diagnostics/*.java : Each diagnostic should have a Java implementation class, resource bundle for localized messages, unit tests, and documentation
📚 Learning: 2025-11-27T07:17:33.726Z
Learnt from: CR
Repo: 1c-syntax/bsl-language-server PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-11-27T07:17:33.726Z
Learning: Applies to docs/*/diagnostics/**/*.md : Update diagnostic documentation in both Russian and English with examples of problematic code and fixes
Applied to files:
docs/diagnostics/UselessTernaryOperator.mdsrc/main/resources/com/github/_1c_syntax/bsl/languageserver/diagnostics/UselessTernaryOperatorDiagnostic_en.propertiessrc/main/resources/com/github/_1c_syntax/bsl/languageserver/configuration/schema.jsonsrc/main/resources/com/github/_1c_syntax/bsl/languageserver/diagnostics/UselessTernaryOperatorDiagnostic_ru.propertiessrc/main/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/UselessTernaryOperatorDiagnostic.javasrc/test/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/UselessTernaryOperatorDiagnosticTest.javadocs/en/diagnostics/UselessTernaryOperator.mdsrc/main/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/DiagnosticStorage.java
📚 Learning: 2024-07-04T19:35:26.747Z
Learnt from: nixel2007
Repo: 1c-syntax/bsl-language-server PR: 3308
File: src/test/resources/diagnostics/DoubleNegativesDiagnostic.bsl:20-21
Timestamp: 2024-07-04T19:35:26.747Z
Learning: The file `DoubleNegativesDiagnostic.bsl` is a test resource intended to demonstrate the functionality of the DoubleNegatives diagnostic.
Applied to files:
docs/diagnostics/UselessTernaryOperator.mdsrc/main/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/UselessTernaryOperatorDiagnostic.javasrc/test/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/UselessTernaryOperatorDiagnosticTest.java
📚 Learning: 2025-11-27T07:17:33.726Z
Learnt from: CR
Repo: 1c-syntax/bsl-language-server PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-11-27T07:17:33.726Z
Learning: Applies to **/diagnostics/*.java : Each diagnostic should have a Java implementation class, resource bundle for localized messages, unit tests, and documentation
Applied to files:
src/main/resources/com/github/_1c_syntax/bsl/languageserver/diagnostics/UselessTernaryOperatorDiagnostic_en.propertiessrc/main/resources/com/github/_1c_syntax/bsl/languageserver/configuration/schema.jsonsrc/main/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/UselessTernaryOperatorDiagnostic.javasrc/test/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/UselessTernaryOperatorDiagnosticTest.javasrc/main/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/DiagnosticStorage.java
📚 Learning: 2025-11-27T07:17:33.726Z
Learnt from: CR
Repo: 1c-syntax/bsl-language-server PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-11-27T07:17:33.726Z
Learning: Applies to **/diagnostics/*.properties : Resource bundles for diagnostics should have both Russian and English versions
Applied to files:
src/main/resources/com/github/_1c_syntax/bsl/languageserver/diagnostics/UselessTernaryOperatorDiagnostic_en.propertiessrc/main/resources/com/github/_1c_syntax/bsl/languageserver/diagnostics/UselessTernaryOperatorDiagnostic_ru.properties
📚 Learning: 2025-11-27T07:17:33.726Z
Learnt from: CR
Repo: 1c-syntax/bsl-language-server PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-11-27T07:17:33.726Z
Learning: Applies to **/diagnostics/*Test.java : Write comprehensive unit tests for each diagnostic including test cases for edge cases, following existing test patterns
Applied to files:
src/main/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/UselessTernaryOperatorDiagnostic.javasrc/test/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/UselessTernaryOperatorDiagnosticTest.javasrc/main/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/DiagnosticStorage.java
📚 Learning: 2025-02-10T17:13:41.452Z
Learnt from: theshadowco
Repo: 1c-syntax/bsl-language-server PR: 3408
File: src/main/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/ProtectedModuleDiagnostic.java:54-58
Timestamp: 2025-02-10T17:13:41.452Z
Learning: In ProtectedModuleDiagnostic.java, the usage of `Range` field as a diagnostic target is accepted as is, despite being marked as a temporary solution in the comments.
Applied to files:
src/main/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/DiagnosticStorage.java
📚 Learning: 2025-11-19T09:02:03.154Z
Learnt from: theshadowco
Repo: 1c-syntax/bsl-language-server PR: 3610
File: src/main/java/com/github/_1c_syntax/bsl/languageserver/folding/QueryPackageFoldingRangeSupplier.java:26-26
Timestamp: 2025-11-19T09:02:03.154Z
Learning: В проекте bsl-language-server используется кастомная версия ANTLR4 от 1c-syntax (io.github.1c-syntax:antlr4), которая включает базовый класс org.antlr.v4.runtime.Tokenizer с методами getAst() и getTokens(). SDBLTokenizer и BSLTokenizer наследуются от этого базового класса.
Applied to files:
src/main/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/DiagnosticStorage.java
📚 Learning: 2025-04-18T22:46:43.245Z
Learnt from: nixel2007
Repo: 1c-syntax/bsl-language-server PR: 3449
File: src/main/java/com/github/_1c_syntax/bsl/languageserver/utils/expressiontree/ExpressionTreeBuildingVisitor.java:192-203
Timestamp: 2025-04-18T22:46:43.245Z
Learning: В проекте bsl-language-server класс BSLParser.ExpressionContext наследуется от BSLParserRuleContext, а не напрямую от ParserRuleContext. При работе с ним нужно учитывать специфичные методы BSLParserRuleContext.
Applied to files:
src/main/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/DiagnosticStorage.java
🧬 Code graph analysis (1)
src/main/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/UselessTernaryOperatorDiagnostic.java (1)
src/main/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/DiagnosticStorage.java (1)
DiagnosticStorage(50-338)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (11)
- GitHub Check: build (21, ubuntu-latest)
- GitHub Check: build (25, windows-latest)
- GitHub Check: build (25, macOS-latest)
- GitHub Check: build (25, ubuntu-latest)
- GitHub Check: build (21, windows-latest)
- GitHub Check: build (17, ubuntu-latest)
- GitHub Check: build (21, macOS-latest)
- GitHub Check: build (17, windows-latest)
- GitHub Check: build (17, macOS-latest)
- GitHub Check: Analyse
- GitHub Check: build
🔇 Additional comments (8)
src/main/resources/com/github/_1c_syntax/bsl/languageserver/diagnostics/UselessTernaryOperatorDiagnostic_ru.properties (1)
1-4: RU diagnostic strings look consistentВсе ключи и формулировки согласованы между собой и с документацией/квикфиксом (
НЕ (%s)), замечаний нет.src/main/resources/com/github/_1c_syntax/bsl/languageserver/configuration/schema.json (1)
494-496: Diagnostic parameter wiring and enum formatting look correctСвойство
UselessTernaryOperatorкорректно ссылается наparameters-schema.json#/definitions/UselessTernaryOperatorпо тому же шаблону, что и остальные диагностики, а измененияenum— чисто форматирование без смены значений. Тут всё в порядке.Also applies to: 930-989
src/main/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/DiagnosticStorage.java (6)
77-86: LGTM!The new overload follows the established pattern correctly, including the
node.exceptionnull-check before proceeding.
112-118: LGTM!Consistent with the existing delegation pattern.
128-135: LGTM!Delegation chain is consistent with existing overloads.
228-245: LGTM!Backward compatibility maintained by passing
nullfor the new data parameter.
296-326: LGTM!The data parameter is properly integrated with a null-check before setting.
327-337: LGTM!The record is appropriately minimal for the current use case, and the JavaDoc clearly documents that this is an intentionally simple implementation.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
docs/diagnostics/UselessTernaryOperator.md (1)
8-23: Clarify the distinction between "Useless" and "Suspicious" operators with explanatory text.The two example groups lack explanation of their differences. Line 5 briefly states the purpose but doesn't clarify:
- What makes a ternary operator "useless" (e.g., equivalent to the condition itself)
- What makes one "suspicious" (e.g., mixing types or asymmetric logic)
- What the corrected patterns should be
Additionally, examples mix Russian and English keywords inconsistently, which reduces clarity in Russian documentation.
Бессмысленные операторы + + Когда тернарный оператор возвращает булевы константы, его можно упростить до самого условия: ```Bsl А = ?(Б = 1, Истина, Ложь); + // Правильно: А = Б = 1;
Подозрительные операторы
Когда значения в ветвях имеют несовместимые типы или странную логику:
-А = ?(Б = 1, True, Истина);
- А = ?(Б = 1, Истина, 0);
```Bsl
- А = ?(Б = 0, 0, False);
- А = ?(Б = 0, Истина, Истина);
This improves clarity by: - Explaining the underlying issue for each category - Adding corrected examples to show the intended fix - Maintaining consistent Russian throughout - Aligning with learnings on comprehensive diagnostic documentation </blockquote></details> </blockquote></details> <details> <summary>📜 Review details</summary> **Configuration used**: Path: .coderabbit.yaml **Review profile**: CHILL **Plan**: Pro <details> <summary>📥 Commits</summary> Reviewing files that changed from the base of the PR and between bc42f49fa4400b50c6a616135d95716bcc8c4989 and 8a3d6def858ad7f303cc709241d1040c1ed65825. </details> <details> <summary>📒 Files selected for processing (2)</summary> * `docs/diagnostics/UselessTernaryOperator.md` (1 hunks) * `docs/en/diagnostics/UselessTernaryOperator.md` (1 hunks) </details> <details> <summary>🚧 Files skipped from review as they are similar to previous changes (1)</summary> * docs/en/diagnostics/UselessTernaryOperator.md </details> <details> <summary>🧰 Additional context used</summary> <details> <summary>📓 Path-based instructions (1)</summary> <details> <summary>**/*.md</summary> **📄 CodeRabbit inference engine (.github/copilot-instructions.md)** > Keep documentation up to date with code changes Files: - `docs/diagnostics/UselessTernaryOperator.md` </details> </details><details> <summary>🧠 Learnings (3)</summary> <details> <summary>📓 Common learnings</summary>Learnt from: CR
Repo: 1c-syntax/bsl-language-server PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-11-27T07:17:33.726Z
Learning: Applies to docs//diagnostics/**/.md : Update diagnostic documentation in both Russian and English with examples of problematic code and fixesLearnt from: CR
Repo: 1c-syntax/bsl-language-server PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-11-27T07:17:33.726Z
Learning: Applies to **/diagnostics/*.java : Each diagnostic should have a Java implementation class, resource bundle for localized messages, unit tests, and documentationLearnt from: CR
Repo: 1c-syntax/bsl-language-server PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-11-27T07:17:33.726Z
Learning: Applies to **/diagnostics/*Test.java : Write comprehensive unit tests for each diagnostic including test cases for edge cases, following existing test patterns</details> <details> <summary>📚 Learning: 2025-11-27T07:17:33.726Z</summary>Learnt from: CR
Repo: 1c-syntax/bsl-language-server PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-11-27T07:17:33.726Z
Learning: Applies to docs//diagnostics/**/.md : Update diagnostic documentation in both Russian and English with examples of problematic code and fixes**Applied to files:** - `docs/diagnostics/UselessTernaryOperator.md` </details> <details> <summary>📚 Learning: 2024-07-04T19:35:26.747Z</summary>Learnt from: nixel2007
Repo: 1c-syntax/bsl-language-server PR: 3308
File: src/test/resources/diagnostics/DoubleNegativesDiagnostic.bsl:20-21
Timestamp: 2024-07-04T19:35:26.747Z
Learning: The fileDoubleNegativesDiagnostic.bslis a test resource intended to demonstrate the functionality of the DoubleNegatives diagnostic.**Applied to files:** - `docs/diagnostics/UselessTernaryOperator.md` </details> </details> </details> <details> <summary>⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (13)</summary> * GitHub Check: build (25, ubuntu-latest) * GitHub Check: build (21, windows-latest) * GitHub Check: build (25, windows-latest) * GitHub Check: build (21, macOS-latest) * GitHub Check: build (25, macOS-latest) * GitHub Check: build (17, ubuntu-latest) * GitHub Check: build (21, ubuntu-latest) * GitHub Check: build (17, windows-latest) * GitHub Check: build (17, macOS-latest) * GitHub Check: build * GitHub Check: Benchmark * GitHub Check: Analyse * GitHub Check: Sentry </details> </details> <!-- This is an auto-generated comment by CodeRabbit for review status -->
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/main/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/DiagnosticStorage.java (2)
286-288: Consider validating the string parameter.The factory method accepts a
Stringparameter but doesn't validate it. Should null strings be rejected, or should the parameter be marked@Nullableto clarify that null is acceptable?If null strings should be rejected, apply this diff:
public static DiagnosticAdditionalData createAdditionalData(String string) { + if (string == null) { + throw new IllegalArgumentException("Additional data string cannot be null"); + } return new DiagnosticAdditionalData(string); }Alternatively, if null is acceptable, mark the parameter as
@Nullable:-public static DiagnosticAdditionalData createAdditionalData(String string) { +public static DiagnosticAdditionalData createAdditionalData(@Nullable String string) { return new DiagnosticAdditionalData(string); }
329-331: Consider a more descriptive parameter name.The record parameter is named
string, which is quite generic. Consider renaming it to something more descriptive likevalue,content, orsupplementalInfoto better convey its purpose.For example:
-public record DiagnosticAdditionalData(String string) { +public record DiagnosticAdditionalData(String value) {Note: The JavaDoc correctly acknowledges this is a primitive implementation for the current task. This simple design is appropriate for the initial use case described in the PR objectives (storing supplemental data for quick fixes).
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/main/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/DiagnosticStorage.java(8 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.java
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.java: Follow the Style Guide provided in docs/en/contributing/StyleGuide.md
Use Lombok annotations to reduce boilerplate code and enable annotation processing in your IDE
Optimize imports before committing but do NOT optimize imports across the entire project unless specifically working on that task
Follow Java naming conventions with meaningful, descriptive names; keep class and method names concise but clear
Write JavaDoc for public APIs and include comments for complex logic
Use Target Java 17 as the language version
Files:
src/main/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/DiagnosticStorage.java
**/diagnostics/*.java
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Each diagnostic should have a Java implementation class, resource bundle for localized messages, unit tests, and documentation
Files:
src/main/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/DiagnosticStorage.java
🧠 Learnings (6)
📓 Common learnings
Learnt from: CR
Repo: 1c-syntax/bsl-language-server PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-11-27T07:17:33.726Z
Learning: Applies to docs/*/diagnostics/**/*.md : Update diagnostic documentation in both Russian and English with examples of problematic code and fixes
Learnt from: CR
Repo: 1c-syntax/bsl-language-server PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-11-27T07:17:33.726Z
Learning: Applies to **/diagnostics/*.java : Each diagnostic should have a Java implementation class, resource bundle for localized messages, unit tests, and documentation
Learnt from: CR
Repo: 1c-syntax/bsl-language-server PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-11-27T07:17:33.726Z
Learning: Applies to **/diagnostics/*Test.java : Write comprehensive unit tests for each diagnostic including test cases for edge cases, following existing test patterns
📚 Learning: 2025-11-27T07:17:33.726Z
Learnt from: CR
Repo: 1c-syntax/bsl-language-server PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-11-27T07:17:33.726Z
Learning: Applies to **/diagnostics/*.java : Each diagnostic should have a Java implementation class, resource bundle for localized messages, unit tests, and documentation
Applied to files:
src/main/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/DiagnosticStorage.java
📚 Learning: 2025-02-10T17:13:41.452Z
Learnt from: theshadowco
Repo: 1c-syntax/bsl-language-server PR: 3408
File: src/main/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/ProtectedModuleDiagnostic.java:54-58
Timestamp: 2025-02-10T17:13:41.452Z
Learning: In ProtectedModuleDiagnostic.java, the usage of `Range` field as a diagnostic target is accepted as is, despite being marked as a temporary solution in the comments.
Applied to files:
src/main/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/DiagnosticStorage.java
📚 Learning: 2025-11-27T07:17:33.726Z
Learnt from: CR
Repo: 1c-syntax/bsl-language-server PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-11-27T07:17:33.726Z
Learning: Applies to docs/*/diagnostics/**/*.md : Update diagnostic documentation in both Russian and English with examples of problematic code and fixes
Applied to files:
src/main/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/DiagnosticStorage.java
📚 Learning: 2025-11-19T09:02:03.154Z
Learnt from: theshadowco
Repo: 1c-syntax/bsl-language-server PR: 3610
File: src/main/java/com/github/_1c_syntax/bsl/languageserver/folding/QueryPackageFoldingRangeSupplier.java:26-26
Timestamp: 2025-11-19T09:02:03.154Z
Learning: В проекте bsl-language-server используется кастомная версия ANTLR4 от 1c-syntax (io.github.1c-syntax:antlr4), которая включает базовый класс org.antlr.v4.runtime.Tokenizer с методами getAst() и getTokens(). SDBLTokenizer и BSLTokenizer наследуются от этого базового класса.
Applied to files:
src/main/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/DiagnosticStorage.java
📚 Learning: 2025-04-18T22:46:43.245Z
Learnt from: nixel2007
Repo: 1c-syntax/bsl-language-server PR: 3449
File: src/main/java/com/github/_1c_syntax/bsl/languageserver/utils/expressiontree/ExpressionTreeBuildingVisitor.java:192-203
Timestamp: 2025-04-18T22:46:43.245Z
Learning: В проекте bsl-language-server класс BSLParser.ExpressionContext наследуется от BSLParserRuleContext, а не напрямую от ParserRuleContext. При работе с ним нужно учитывать специфичные методы BSLParserRuleContext.
Applied to files:
src/main/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/DiagnosticStorage.java
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (12)
- GitHub Check: build (25, macOS-latest)
- GitHub Check: build (17, macOS-latest)
- GitHub Check: build (21, windows-latest)
- GitHub Check: build (25, windows-latest)
- GitHub Check: build (17, windows-latest)
- GitHub Check: build (25, ubuntu-latest)
- GitHub Check: build (21, macOS-latest)
- GitHub Check: build (21, ubuntu-latest)
- GitHub Check: build (17, ubuntu-latest)
- GitHub Check: Analyse
- GitHub Check: build
- GitHub Check: Analyze the repo with CodeSee
🔇 Additional comments (2)
src/main/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/DiagnosticStorage.java (2)
77-86: LGTM! Well-integrated overloads for diagnostic data.The new
addDiagnosticoverloads properly follow existing patterns, maintain exception/empty-range checks, delegate correctly through the method chain, and preserve thread safety and backward compatibility.Also applies to: 112-118, 128-135, 241-259
290-320: LGTM! Proper null-safe handling of additional data.The method correctly accepts
@Nullable DiagnosticAdditionalData, only callssetDatawhen the data is not null, and integrates well with the LSP4j Diagnostic API for use in code action requests.
|
Описание
Связанные задачи
Closes #203
Чеклист
Общие
gradlew precommit)Для диагностик
Дополнительно
Это переработка заброшенного реквеста #1820
Summary by CodeRabbit
New Features
Configuration
Documentation
Localization
Tests
✏️ Tip: You can customize this high-level summary in your review settings.