Merged
Conversation
WalkthroughОбновлены сборка и зависимости (SonarQube → 25.4, commonmark, JUnit/Mockito), переход на Java 21; README дополнил строку про 25.4+; в код добавлены Lombok Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Tip Issue Planner is now in beta. Read the docs and try it out! Share your feedback on Discord. 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.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/main/java/com/github/_1c_syntax/bsl/sonar/BSLCoreSensor.java (1)
163-170:⚠️ Potential issue | 🟡 MinorПредсуществующее состояние гонки при параллельной обработке файлов.
count[0]++вparallelStream().forEach()выполняется несколькими потоками без синхронизации — операция чтения-изменения-записи неатомарна. В результате счётчик может терять инкременты, а прогресс-сообщения печататься нерегулярно. Это предсуществующая проблема, не введённая данным PR (изменение только добавляет константуCOUNT_FILES_PB), однако стоит её исправить.🔒 Предлагаемое исправление: использовать `AtomicInteger`
- int total = inputFilesList.size(); - final int[] count = {0}; + int total = inputFilesList.size(); + final var count = new java.util.concurrent.atomic.AtomicInteger(0); inputFilesList.parallelStream().forEach((InputFile inputFile) -> { var uri = inputFile.uri(); LOGGER.debug(uri.toString()); processFile(inputFile, bslServerContext); - count[0]++; - if (count[0] % COUNT_FILES_PB == 0) { - LOGGER.info("Processing files: {}/{}", count[0], total); + int current = count.incrementAndGet(); + if (current % COUNT_FILES_PB == 0) { + LOGGER.info("Processing files: {}/{}", current, total); } }); - LOGGER.info("Processing files: {}/{}", count[0], total); + LOGGER.info("Processing files: {}/{}", count.get(), total);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/com/github/_1c_syntax/bsl/sonar/BSLCoreSensor.java` around lines 163 - 170, The parallel forEach uses a non-atomic mutable counter (count[0]) causing a race and lost increments; replace it with an AtomicInteger (e.g., AtomicInteger processed = new AtomicInteger()) and update usages in the lambda (use processed.incrementAndGet() or processed.getAndIncrement() and compare to COUNT_FILES_PB) so progress logging in BSLCoreSensor (inside the inputFilesList.parallelStream().forEach where processFile(...) is called and LOGGER.info(...) is emitted) is thread-safe and accurate.
🧹 Nitpick comments (2)
lombok.config (1)
1-3: Рекомендуется добавитьconfig.stopBubbling = true.Конфигурационные файлы Lombok являются иерархическими: настройки применяются ко всем исходным файлам в текущей директории и всех поддиректориях. Чтобы Lombok не поднимался по родительским директориям в поисках дополнительных конфигурационных файлов, в корневой конфиг рекомендуется добавить
config.stopBubbling = true.♻️ Предлагаемое дополнение
+config.stopBubbling=true lombok.anyConstructor.addConstructorProperties=true lombok.addLombokGeneratedAnnotation=true lombok.log.fieldName=LOGGER🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lombok.config` around lines 1 - 3, Add the Lombok config directive to stop upward inheritance so parent directories' lombok.config won't affect this project: add the property config.stopBubbling = true to the existing lombok.config (which already contains lombok.anyConstructor.addConstructorProperties, lombok.addLombokGeneratedAnnotation, and lombok.log.fieldName=LOGGER) so the configuration is self-contained.build.gradle.kts (1)
18-23: Дублирующаяся запись репозитория Maven Central.
maven("https://repo.maven.apache.org/maven2")на строке 22 идентична тому, на что указываетmavenCentral()в Gradle. Это приводит к тому, что Gradle дважды запрашивает один и тот же репозиторий при разрешении зависимостей, незначительно замедляя сборку.♻️ Предлагаемое исправление
repositories { mavenLocal() mavenCentral() maven("https://central.sonatype.com/repository/maven-snapshots") - maven("https://repo.maven.apache.org/maven2") }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@build.gradle.kts` around lines 18 - 23, В блоке repositories убрана дублирующая запись Maven Central: удалите одну из двух идентичных записей (либо вызов mavenCentral(), либо maven("https://repo.maven.apache.org/maven2")) чтобы избежать двойного запроса к одному и тому же репозиторию; измените только удаление дублирующей строки в том же блоке repositories (оставьте либо mavenCentral(), либо maven("https://repo.maven.apache.org/maven2")) и сохраните остальные репозитории (mavenLocal() и snapshots) без изменений.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@src/main/java/com/github/_1c_syntax/bsl/sonar/BSLCoreSensor.java`:
- Around line 163-170: The parallel forEach uses a non-atomic mutable counter
(count[0]) causing a race and lost increments; replace it with an AtomicInteger
(e.g., AtomicInteger processed = new AtomicInteger()) and update usages in the
lambda (use processed.incrementAndGet() or processed.getAndIncrement() and
compare to COUNT_FILES_PB) so progress logging in BSLCoreSensor (inside the
inputFilesList.parallelStream().forEach where processFile(...) is called and
LOGGER.info(...) is emitted) is thread-safe and accurate.
---
Nitpick comments:
In `@build.gradle.kts`:
- Around line 18-23: В блоке repositories убрана дублирующая запись Maven
Central: удалите одну из двух идентичных записей (либо вызов mavenCentral(),
либо maven("https://repo.maven.apache.org/maven2")) чтобы избежать двойного
запроса к одному и тому же репозиторию; измените только удаление дублирующей
строки в том же блоке repositories (оставьте либо mavenCentral(), либо
maven("https://repo.maven.apache.org/maven2")) и сохраните остальные репозитории
(mavenLocal() и snapshots) без изменений.
In `@lombok.config`:
- Around line 1-3: Add the Lombok config directive to stop upward inheritance so
parent directories' lombok.config won't affect this project: add the property
config.stopBubbling = true to the existing lombok.config (which already contains
lombok.anyConstructor.addConstructorProperties,
lombok.addLombokGeneratedAnnotation, and lombok.log.fieldName=LOGGER) so the
configuration is self-contained.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/main/java/com/github/_1c_syntax/bsl/sonar/ext_issues/RulesFileReader.java (1)
78-78:⚠️ Potential issue | 🔴 CriticalКритическая ошибка компиляции:
LOGGERне существует после добавления аннотации@Slf4jАннотация
@Slf4j(Lombok) генерирует полеprivate static final Logger log, а неLOGGER. Явное объявлениеLOGGERбыло удалено, но три вызова логирования на строках 78, 96 и 118 по-прежнему ссылаются на этот несуществующий символ. Код не скомпилируется.Требуется заменить все три вызова
LOGGERнаlog:Исправления
// строка 78 - LOGGER.error("Can't read json file rules", e); + log.error("Can't read json file rules", e); // строка 96 - LOGGER.error("Can't serialize json rules to object", e); + log.error("Can't serialize json rules to object", e); // строка 118 - LOGGER.error("Can't read json file rules {}", file.toURI(), e); + log.error("Can't read json file rules {}", file.toURI(), e);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/com/github/_1c_syntax/bsl/sonar/ext_issues/RulesFileReader.java` at line 78, The code uses the removed LOGGER field after adding Lombok's `@Slf4j` (which generates a private static final Logger named log); update all logging calls in RulesFileReader that reference LOGGER (specifically the calls on lines shown, e.g., the one currently written as LOGGER.error("Can't read json file rules", e)) to use the Lombok-generated log identifier (replace LOGGER with log) so the class compiles and logging works.
🧹 Nitpick comments (1)
build.gradle.kts (1)
44-44: Рекомендуется вынести версию JUnit в отдельную переменнуюВерсия
"6.0.3"продублирована в трёх местах:junit-jupiter-api(Line 44),junit-jupiter-engine(Line 55),junit-platform-launcher(Line 56). По аналогии с уже введённымиsonarQubeVersionиcommonmarkVersion, стоит централизовать версию JUnit.♻️ Предлагаемый рефакторинг
val sonarQubeVersion = "25.4.0.105899" val commonmarkVersion = "0.27.1" +val junitVersion = "6.0.3" ... - testImplementation("org.junit.jupiter", "junit-jupiter-api", "6.0.3") + testImplementation("org.junit.jupiter", "junit-jupiter-api", junitVersion) ... - testRuntimeOnly("org.junit.jupiter", "junit-jupiter-engine", "6.0.3") - testRuntimeOnly("org.junit.platform", "junit-platform-launcher", "6.0.3") + testRuntimeOnly("org.junit.jupiter", "junit-jupiter-engine", junitVersion) + testRuntimeOnly("org.junit.platform", "junit-platform-launcher", junitVersion)Also applies to: 55-56
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@build.gradle.kts` at line 44, Вынести строковую версию JUnit в отдельную переменную (например val junitVersion = "6.0.3") и заменить все вхождения литерала "6.0.3" в зависимостях на эту переменную; конкретно обновить вызовы testImplementation("org.junit.jupiter","junit-jupiter-api", "6.0.3"), testRuntimeOnly("org.junit.jupiter","junit-jupiter-engine", "6.0.3") и testImplementation("org.junit.platform","junit-platform-launcher", "6.0.3") чтобы использовать junitVersion вместо литерала, сохраняя существующий стиль объявления версий (как для sonarQubeVersion и commonmarkVersion).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In
`@src/main/java/com/github/_1c_syntax/bsl/sonar/ext_issues/RulesFileReader.java`:
- Line 78: The code uses the removed LOGGER field after adding Lombok's `@Slf4j`
(which generates a private static final Logger named log); update all logging
calls in RulesFileReader that reference LOGGER (specifically the calls on lines
shown, e.g., the one currently written as LOGGER.error("Can't read json file
rules", e)) to use the Lombok-generated log identifier (replace LOGGER with log)
so the class compiles and logging works.
---
Nitpick comments:
In `@build.gradle.kts`:
- Line 44: Вынести строковую версию JUnit в отдельную переменную (например val
junitVersion = "6.0.3") и заменить все вхождения литерала "6.0.3" в зависимостях
на эту переменную; конкретно обновить вызовы
testImplementation("org.junit.jupiter","junit-jupiter-api", "6.0.3"),
testRuntimeOnly("org.junit.jupiter","junit-jupiter-engine", "6.0.3") и
testImplementation("org.junit.platform","junit-platform-launcher", "6.0.3")
чтобы использовать junitVersion вместо литерала, сохраняя существующий стиль
объявления версий (как для sonarQubeVersion и commonmarkVersion).
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/test/java/com/github/_1c_syntax/bsl/sonar/LanguageServerDiagnosticsLoaderSensorTest.java (1)
83-110: Единственная проверка вtest_error— только smoke-тест на отсутствие краша.
assertThat(context.isCancelled()).isFalse()лишь подтверждает, что сенсор не упал. Для теста на обработку ошибок это недостаточно: не проверяется ни то, что issues не были добавлены по невалидным путям, ни то, что количество обработанных отчётов соответствует ожидаемому. Рекомендуется добавить хотя бы проверку на количество репортёров или отсутствие issues.Кроме того, имя
test_errorне раскрывает сценарий — более информативным было бы, например,test_execute_with_invalid_report_paths.♻️ Пример более явных assertions
assertThat(context.isCancelled()).isFalse(); + // Нет issues по невалидным/несуществующим путям отчётов + assertThat(context.allIssues()).isEmpty();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/test/java/com/github/_1c_syntax/bsl/sonar/LanguageServerDiagnosticsLoaderSensorTest.java` around lines 83 - 110, Rename the test method test_error to a descriptive name like test_execute_with_invalid_report_paths and augment it to assert meaningful outcomes: after calling LanguageServerDiagnosticsLoaderSensor.execute(context) verify (1) that no issues were added to the SensorContextTester for the invalid paths (e.g., assert zero issues for inputFile or context.issues()), and (2) that the number of processed report paths matches expectations given the settings key "sonar.bsl.languageserver.reportPaths" (for example check a reporter count field or a public accessor on LanguageServerDiagnosticsLoaderSensor or inspect side-effects indicating how many reports were attempted); keep the existing smoke assertion context.isCancelled() for completeness. Ensure you reference the existing setup (context.settings().setProperty(...), context.fileSystem().add(inputFile), and activeRules) and update the test name and assertions accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@src/test/java/com/github/_1c_syntax/bsl/sonar/LanguageServerDiagnosticsLoaderSensorTest.java`:
- Around line 86-87: Тест у LanguageServerDiagnosticsLoaderSensorTest создает
переменную fileName = "src/test6.bsl" и вызывает Tools.inputFileBSL, но сам
тестовый файл отсутствует, поэтому Tools.inputFileBSL подставляет дефолтный
контент; добавьте реальный тестовый файл src/test6.bsl в src/test/resources с
ожидаемым содержимым, используемым в проверках (соответствующим намерениям
теста), чтобы тест использовал реальный файл вместо дефолтного значения
Tools.inputFileBSL.
---
Nitpick comments:
In
`@src/test/java/com/github/_1c_syntax/bsl/sonar/LanguageServerDiagnosticsLoaderSensorTest.java`:
- Around line 83-110: Rename the test method test_error to a descriptive name
like test_execute_with_invalid_report_paths and augment it to assert meaningful
outcomes: after calling LanguageServerDiagnosticsLoaderSensor.execute(context)
verify (1) that no issues were added to the SensorContextTester for the invalid
paths (e.g., assert zero issues for inputFile or context.issues()), and (2) that
the number of processed report paths matches expectations given the settings key
"sonar.bsl.languageserver.reportPaths" (for example check a reporter count field
or a public accessor on LanguageServerDiagnosticsLoaderSensor or inspect
side-effects indicating how many reports were attempted); keep the existing
smoke assertion context.isCancelled() for completeness. Ensure you reference the
existing setup (context.settings().setProperty(...),
context.fileSystem().add(inputFile), and activeRules) and update the test name
and assertions accordingly.
src/test/java/com/github/_1c_syntax/bsl/sonar/LanguageServerDiagnosticsLoaderSensorTest.java
Show resolved
Hide resolved
nixel2007
reviewed
Feb 18, 2026
src/main/java/com/github/_1c_syntax/bsl/sonar/ext_issues/RulesFileReader.java
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
src/main/java/com/github/_1c_syntax/bsl/sonar/language/BSLLanguageServerRuleDefinition.java (2)
274-291: МаппингPERFORMANCE→MAINTAINABILITY— стоит рассмотретьRELIABILITYилиSECURITY.
PERFORMANCE(строка 284) сопоставлен сSoftwareQuality.MAINTAINABILITY. В контексте SonarQube, проблемы производительности чаще ассоциируются сRELIABILITY. Это не баг, но может не совпадать с ожидаемым поведением пользователей при фильтрации по качеству.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/com/github/_1c_syntax/bsl/sonar/language/BSLLanguageServerRuleDefinition.java` around lines 274 - 291, In createImpactsMap(), change the mapping for DiagnosticTag.PERFORMANCE so it uses a more appropriate SoftwareQuality (e.g., RELIABILITY or SECURITY) instead of MAINTAINABILITY; locate the map.put(DiagnosticTag.PERFORMANCE, Pair.of(SoftwareQuality.MAINTAINABILITY, Severity.MEDIUM)) entry and update the Pair.of to use SoftwareQuality.RELIABILITY (or SECURITY) with the same Severity if desired.
125-125: Нит: можно использовать ссылку на метод вместо лямбды.♻️ Предлагаемое упрощение
- .map((DiagnosticCode diagnosticCode) -> diagnosticCode.getStringValue()) + .map(DiagnosticCode::getStringValue)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/com/github/_1c_syntax/bsl/sonar/language/BSLLanguageServerRuleDefinition.java` at line 125, Replace the explicit lambda in the stream mapping with a method reference to simplify the code: change the mapping expression .map((DiagnosticCode diagnosticCode) -> diagnosticCode.getStringValue()) in BSLLanguageServerRuleDefinition to use DiagnosticCode::getStringValue so the stream pipeline is more concise and idiomatic.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@src/main/java/com/github/_1c_syntax/bsl/sonar/language/BSLLanguageServerRuleDefinition.java`:
- Around line 61-72: The code currently iterates diagnostic tags and calls
addDefaultImpact for each tag which can cause duplicate SoftwareQuality entries
(from IMPACTS_MAP) and IllegalStateException; change the logic in the
rule-building section that processes diagnostic tags so you first map tags →
(SoftwareQuality, Severity) via IMPACTS_MAP, then deduplicate by SoftwareQuality
keeping the highest-priority Severity for each SoftwareQuality, and only then
call addDefaultImpact once per SoftwareQuality; also after mapping diagnostic
tags to CLEAN_CODE_ATTRIBUTE_MAP add a filter(Objects::nonNull) to avoid
potential NPEs when a tag is unmapped; update references to addDefaultImpact,
IMPACTS_MAP and CLEAN_CODE_ATTRIBUTE_MAP to locate the changes.
- Around line 153-168: The code may add duplicate impacts for the same
SoftwareQuality causing IllegalStateException; change the impact population to
deduplicate by SoftwareQuality before calling newRule.addDefaultImpact: when
iterating diagnosticInfo.getTags() use IMPACTS_MAP.get(tag), filter out nulls,
map to the impact pair and then use a grouping/deduplication by impact.getLeft()
(SoftwareQuality) so you only call addDefaultImpact once per unique
SoftwareQuality, and likewise update the CLEAN_CODE_ATTRIBUTE_MAP handling (used
in diagnosticInfo.getTags().stream() ... newRule.setCleanCodeAttribute) to
filter out null results with filter(Objects::nonNull) before
distinct/sorted/findFirst to avoid NPEs when a tag is unmapped; keep references
to diagnosticInfo, CLEAN_CODE_ATTRIBUTE_MAP, newRule.setCleanCodeAttribute,
IMPACTS_MAP and newRule.addDefaultImpact when locating the changes.
---
Nitpick comments:
In
`@src/main/java/com/github/_1c_syntax/bsl/sonar/language/BSLLanguageServerRuleDefinition.java`:
- Around line 274-291: In createImpactsMap(), change the mapping for
DiagnosticTag.PERFORMANCE so it uses a more appropriate SoftwareQuality (e.g.,
RELIABILITY or SECURITY) instead of MAINTAINABILITY; locate the
map.put(DiagnosticTag.PERFORMANCE, Pair.of(SoftwareQuality.MAINTAINABILITY,
Severity.MEDIUM)) entry and update the Pair.of to use
SoftwareQuality.RELIABILITY (or SECURITY) with the same Severity if desired.
- Line 125: Replace the explicit lambda in the stream mapping with a method
reference to simplify the code: change the mapping expression
.map((DiagnosticCode diagnosticCode) -> diagnosticCode.getStringValue()) in
BSLLanguageServerRuleDefinition to use DiagnosticCode::getStringValue so the
stream pipeline is more concise and idiomatic.
src/main/java/com/github/_1c_syntax/bsl/sonar/language/BSLLanguageServerRuleDefinition.java
Outdated
Show resolved
Hide resolved
src/main/java/com/github/_1c_syntax/bsl/sonar/language/BSLLanguageServerRuleDefinition.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@src/main/java/com/github/_1c_syntax/bsl/sonar/language/BSLLanguageServerRuleDefinition.java`:
- Around line 264-300: createCleanCodeAttributeMap() and createImpactsMap()
currently hard-code entries for DiagnosticTag and will silently miss new enum
constants; iterate DiagnosticTag.values() and validate coverage instead of
relying on manual entries: build the maps (or an EnumMap) as now, then compute
missing = EnumSet.allOf(DiagnosticTag.class) minus map.keySet() and if missing
is non-empty throw an IllegalStateException listing the unknown/missing
DiagnosticTag values so the failure is loud; also add a unit test that asserts
no missing tags for both createCleanCodeAttributeMap() and createImpactsMap() to
prevent regressions.
---
Duplicate comments:
In
`@src/main/java/com/github/_1c_syntax/bsl/sonar/language/BSLLanguageServerRuleDefinition.java`:
- Around line 157-162: The stream mapping diagnosticInfo.getTags() via
CLEAN_CODE_ATTRIBUTE_MAP::get can produce nulls which cause NPE during
.sorted(); update the pipeline to filter out null mapping results before sorting
(e.g.,
.map(CLEAN_CODE_ATTRIBUTE_MAP::get).filter(Objects::nonNull).distinct().sorted().findFirst().ifPresent(newRule::setCleanCodeAttribute))
and add import java.util.Objects if missing; ensure you still call
newRule::setCleanCodeAttribute only with non-null values.
- Around line 211-221: computeImpact currently calls IMPACTS_MAP.get(tag) and
immediately dereferences the result, causing an NPE for tags not present in
IMPACTS_MAP; update computeImpact to null-check the result of
IMPACTS_MAP.get(tag) (the local variable impact) and skip processing that tag
(or handle a default) when impact is null before calling
impact.getLeft()/getRight(), leaving the rest of the logic (map.get(...),
map.put(...)) unchanged; refer to computeImpact, DiagnosticTag, and
IMPACTS_MAP.get(tag) to find where to add the null check.
src/main/java/com/github/_1c_syntax/bsl/sonar/language/BSLLanguageServerRuleDefinition.java
Outdated
Show resolved
Hide resolved
theshadowco
force-pushed
the
feature/min20254
branch
from
230b287 to
18b1ada
Compare
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/main/java/com/github/_1c_syntax/bsl/sonar/BSLCoreSensor.java (2)
411-417:⚠️ Potential issue | 🔴 Critical
HashMap.newHashMap(...)не существует вjava.util.HashMapи вызовет ошибку компиляции.Замените на конструктор
new HashMap<>(...).Исправление
- Map<String, Object> diagnosticConfiguration = HashMap.newHashMap(diagnosticParameters.size()); + Map<String, Object> diagnosticConfiguration = new HashMap<>(diagnosticParameters.size());🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/com/github/_1c_syntax/bsl/sonar/BSLCoreSensor.java` around lines 411 - 417, The code in getDiagnosticConfiguration uses non-existent HashMap.newHashMap(...) which will not compile; replace that call with the standard constructor new HashMap<>(diagnosticParameters.size()) when creating diagnosticConfiguration (refer to getDiagnosticConfiguration, DiagnosticInfo.getParameters(), ActiveRule.params(), and the diagnosticConfiguration variable) so the map is instantiated correctly.
36-66:⚠️ Potential issue | 🔴 CriticalИсправить вызов конструктора HashMap на строке 416.
HashMap.newHashMap()— некорректный синтаксис. Классjava.util.HashMapне имеет статического методаnewHashMap(). Используйте конструктор:Map<String, Object> diagnosticConfiguration = new HashMap<>(diagnosticParameters.size());Изменение на
@Slf4jи конфигурация Lombok (версия 9.2.0) применены корректно. КонстантаCOUNT_FILES_PBдобавлена правильно.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/com/github/_1c_syntax/bsl/sonar/BSLCoreSensor.java` around lines 36 - 66, Replace the incorrect static factory call HashMap.newHashMap() with a proper constructor when creating the Map for diagnosticConfiguration in class BSLCoreSensor: instantiate a new java.util.HashMap with an initial capacity (e.g. new HashMap<>(diagnosticParameters.size())) so the variable diagnosticConfiguration is created correctly; locate the creation site referenced in the review and update it to use the HashMap constructor instead of the non-existent static method.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@src/main/java/com/github/_1c_syntax/bsl/sonar/BSLCoreSensor.java`:
- Around line 411-417: The code in getDiagnosticConfiguration uses non-existent
HashMap.newHashMap(...) which will not compile; replace that call with the
standard constructor new HashMap<>(diagnosticParameters.size()) when creating
diagnosticConfiguration (refer to getDiagnosticConfiguration,
DiagnosticInfo.getParameters(), ActiveRule.params(), and the
diagnosticConfiguration variable) so the map is instantiated correctly.
- Around line 36-66: Replace the incorrect static factory call
HashMap.newHashMap() with a proper constructor when creating the Map for
diagnosticConfiguration in class BSLCoreSensor: instantiate a new
java.util.HashMap with an initial capacity (e.g. new
HashMap<>(diagnosticParameters.size())) so the variable diagnosticConfiguration
is created correctly; locate the creation site referenced in the review and
update it to use the HashMap constructor instead of the non-existent static
method.
There was a problem hiding this comment.
🧹 Nitpick comments (3)
src/test/java/com/github/_1c_syntax/bsl/sonar/ext_issues/RuleDefinitionTest.java (2)
96-98: Непоследовательный способ построения путей к файламВ
testExternalFile(строка 72) используетсяnew File("src/test/resources").getAbsoluteFile()для явного получения абсолютного пути базовой директории перед построением дочерних путей. В новом тестеFile-объекты создаются напрямую из относительных строк без предварительного вызоваgetAbsoluteFile(). ВызовgetAbsolutePath()при передаче в свойство даёт аналогичный результат в среде Maven, однако подход отличается от принятого в соседних тестах.♻️ Предлагаемый рефакторинг для единообразия
void testExternalFileWithErrors() { // все должно отработать, несмотря на отсутствующие и битые файлы - var fakeRulePath = new File("fake.json"); - var noRulePath = new File("src/test/resources/examples/.bsl-language-server.json"); - var noConfigPath = new File("src/test/resources/examples/highlight.bsl"); + var baseDir = new File("src/test/resources").getAbsoluteFile(); + var fakeRulePath = new File("fake.json").getAbsoluteFile(); + var noRulePath = new File(baseDir, "examples/.bsl-language-server.json"); + var noConfigPath = new File(baseDir, "examples/highlight.bsl"); var config = new MapSettings() .setProperty(reporter.getEnabledKey(), true) .setProperty(reporter.getRulesPathsKey(), fakeRulePath.getAbsolutePath()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/test/java/com/github/_1c_syntax/bsl/sonar/ext_issues/RuleDefinitionTest.java` around lines 96 - 98, Tests use inconsistent path construction: replace the direct relative File constructions for fakeRulePath, noRulePath, and noConfigPath with the same pattern used in testExternalFile. Create a base directory with new File("src/test/resources").getAbsoluteFile() (or reuse the existing base variable) and then construct fakeRulePath, noRulePath, and noConfigPath via new File(base, "relative/path") so all tests build child paths from the same absolute base.
49-50: Вынести повторяющееся магическое число в именованную константуЗначение
281(базовое количество правил) используется в трёх тестах (testDefine,testEmptyExternalFilePath,testExternalFileWithErrors). При изменении набора базовых правил придётся обновлять все три места. Аналогично для285вtestExternalFile.♻️ Предлагаемый рефакторинг
class RuleDefinitionTest { private final Reporter reporter = EdtReporter.create(); + private static final int BASE_RULES_COUNT = 281; + private static final int EXTENDED_RULES_COUNT = 285; `@Test` void testDefine() { ... - assertThat(repository.rules()).hasSize(281); + assertThat(repository.rules()).hasSize(BASE_RULES_COUNT);void testEmptyExternalFilePath() { ... - assertThat(repository.rules()).hasSize(281); + assertThat(repository.rules()).hasSize(BASE_RULES_COUNT);void testExternalFile() { ... - assertThat(repository.rules()).hasSize(285); + assertThat(repository.rules()).hasSize(EXTENDED_RULES_COUNT);void testExternalFileWithErrors() { ... - assertThat(repository.rules()).hasSize(281); + assertThat(repository.rules()).hasSize(BASE_RULES_COUNT);Also applies to: 66-67, 112-113
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/test/java/com/github/_1c_syntax/bsl/sonar/ext_issues/RuleDefinitionTest.java` around lines 49 - 50, Extract the magic numbers into named constants in RuleDefinitionTest: introduce private static final int BASE_RULE_COUNT = 281 and private static final int BASE_RULE_COUNT_WITH_EXTERNAL = 285 (or similarly named constants) and replace literal usages in the test methods testDefine, testEmptyExternalFilePath, testExternalFileWithErrors (and testExternalFile) and their assertions (the lines asserting repository.rules().hasSize(...)) to use these constants; update any other occurrences (lines referenced 49-50, 66-67, 112-113) to reference the constants so future changes to the base rule set require only a single constant update.src/main/java/com/github/_1c_syntax/bsl/sonar/BSLCoreSensor.java (1)
163-170:count[0]++не является атомарной операцией вparallelStream.
count[0]++— это составная операция чтение–изменение–запись без синхронизации. Одновременное выполнение несколькими потоками приводит к потере обновлений. Поскольку это индикатор прогресса, практическое влияние минимально, но для корректности стоит использоватьAtomicInteger.♻️ Предлагаемый рефакторинг
- final int[] count = {0}; + var count = new AtomicInteger(0); inputFilesList.parallelStream().forEach((InputFile inputFile) -> { var uri = inputFile.uri(); LOGGER.debug(uri.toString()); processFile(inputFile, bslServerContext); - count[0]++; - if (count[0] % COUNT_FILES_PB == 0) { - LOGGER.info("Processing files: {}/{}", count[0], total); + var current = count.incrementAndGet(); + if (current % COUNT_FILES_PB == 0) { + LOGGER.info("Processing files: {}/{}", current, total); } }); - LOGGER.info("Processing files: {}/{}", count[0], total); + LOGGER.info("Processing files: {}/{}", count.get(), total);Потребуется добавить импорт
java.util.concurrent.atomic.AtomicInteger.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/com/github/_1c_syntax/bsl/sonar/BSLCoreSensor.java` around lines 163 - 170, Replace the non-atomic progress counter used inside inputFilesList.parallelStream() — specifically the count[0]++ update — with a thread-safe AtomicInteger: add import java.util.concurrent.atomic.AtomicInteger, initialize an AtomicInteger (e.g., AtomicInteger processed = new AtomicInteger(0)) in place of the count array, use processed.incrementAndGet() when a file is processed, and use the returned int for the modulo check (processed.get() % COUNT_FILES_PB or the incrementAndGet() result) when logging from processFile/inputFilesList.parallelStream() so concurrent updates are correct.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/main/java/com/github/_1c_syntax/bsl/sonar/BSLCoreSensor.java`:
- Around line 163-170: Replace the non-atomic progress counter used inside
inputFilesList.parallelStream() — specifically the count[0]++ update — with a
thread-safe AtomicInteger: add import java.util.concurrent.atomic.AtomicInteger,
initialize an AtomicInteger (e.g., AtomicInteger processed = new
AtomicInteger(0)) in place of the count array, use processed.incrementAndGet()
when a file is processed, and use the returned int for the modulo check
(processed.get() % COUNT_FILES_PB or the incrementAndGet() result) when logging
from processFile/inputFilesList.parallelStream() so concurrent updates are
correct.
In
`@src/test/java/com/github/_1c_syntax/bsl/sonar/ext_issues/RuleDefinitionTest.java`:
- Around line 96-98: Tests use inconsistent path construction: replace the
direct relative File constructions for fakeRulePath, noRulePath, and
noConfigPath with the same pattern used in testExternalFile. Create a base
directory with new File("src/test/resources").getAbsoluteFile() (or reuse the
existing base variable) and then construct fakeRulePath, noRulePath, and
noConfigPath via new File(base, "relative/path") so all tests build child paths
from the same absolute base.
- Around line 49-50: Extract the magic numbers into named constants in
RuleDefinitionTest: introduce private static final int BASE_RULE_COUNT = 281 and
private static final int BASE_RULE_COUNT_WITH_EXTERNAL = 285 (or similarly named
constants) and replace literal usages in the test methods testDefine,
testEmptyExternalFilePath, testExternalFileWithErrors (and testExternalFile) and
their assertions (the lines asserting repository.rules().hasSize(...)) to use
these constants; update any other occurrences (lines referenced 49-50, 66-67,
112-113) to reference the constants so future changes to the base rule set
require only a single constant update.
f239486 to
89e40fc
Compare
theshadowco
force-pushed
the
feature/min20254
branch
from
fbef3b2 to
78cb3fe
Compare
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/test/java/com/github/_1c_syntax/bsl/sonar/ext_issues/RuleDefinitionTest.java (1)
93-114: Опциональный рефакторинг: вынести магическое число 281 в константуЗначение
281повторяется в трёх тестах (testDefine,testEmptyExternalFilePath,testExternalFileWithErrors). При изменении набора встроенных правил придётся обновлять все три места вручную. Можно вынести в статическую константу уровня класса.♻️ Предлагаемый рефакторинг
class RuleDefinitionTest { + private static final int DEFAULT_RULES_COUNT = 281; + private final Reporter reporter = EdtReporter.create();- assertThat(repository.rules()).hasSize(281); + assertThat(repository.rules()).hasSize(DEFAULT_RULES_COUNT);(применить во всех трёх тестах)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/test/java/com/github/_1c_syntax/bsl/sonar/ext_issues/RuleDefinitionTest.java` around lines 93 - 114, Tests in RuleDefinitionTest use the magic number 281 in multiple places (testDefine, testEmptyExternalFilePath, testExternalFileWithErrors); extract this into a single static class-level constant (e.g., EXPECTED_RULE_COUNT or DEFAULT_RULE_COUNT) and replace the literal 281 in all three test methods with that constant so updates to the expected built-in rule count are made in one place.build.gradle.kts (1)
60-61: Опционально: рекомендуетсяreleaseвместоsourceCompatibility/targetCompatibility.Флаги
sourceCompatibility/targetCompatibilityне запрещают ссылки на API более новой JDK, чем целевая. Флаг--release 21обеспечивает строгую проверку cross-compilation границ.♻️ Предлагаемое изменение
tasks.withType<JavaCompile> { options.encoding = "UTF-8" + options.release.set(21) options.compilerArgs.add("-Xlint:unchecked") }После этого блоки
java { sourceCompatibility / targetCompatibility }можно убрать, посколькуreleaseих заменяет.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@build.gradle.kts` around lines 60 - 61, Замените установки sourceCompatibility/targetCompatibility (JavaVersion.VERSION_21) на использование флага --release: удалите или уберите sourceCompatibility и targetCompatibility и добавьте настройку compilera, например tasks.withType<JavaCompile> { options.release.set(21) } (или эквивалент в вашем Gradle Kotlin DSL), чтобы обеспечить строгую проверку cross-compilation границ; используйте options.release (или аналогичный параметр для других задач компиляции) вместо sourceCompatibility/targetCompatibility.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@src/test/java/com/github/_1c_syntax/bsl/sonar/ext_issues/RuleDefinitionTest.java`:
- Around line 96-98: Add explicit existence assertions for the test fixture
files so the test fails fast if resources were removed: before configuring the
variables fakeRulePath, noRulePath, and noConfigPath in RuleDefinitionTest
(variables named fakeRulePath, noRulePath, noConfigPath), assert that
noRulePath.exists() and noConfigPath.exists() (and optionally
fakeRulePath.exists() if relevant) and fail with a clear message; this ensures
the test still validates the intended "missing + two broken" scenario rather
than silently operating on non-existent files.
---
Nitpick comments:
In `@build.gradle.kts`:
- Around line 60-61: Замените установки sourceCompatibility/targetCompatibility
(JavaVersion.VERSION_21) на использование флага --release: удалите или уберите
sourceCompatibility и targetCompatibility и добавьте настройку compilera,
например tasks.withType<JavaCompile> { options.release.set(21) } (или эквивалент
в вашем Gradle Kotlin DSL), чтобы обеспечить строгую проверку cross-compilation
границ; используйте options.release (или аналогичный параметр для других задач
компиляции) вместо sourceCompatibility/targetCompatibility.
In
`@src/test/java/com/github/_1c_syntax/bsl/sonar/ext_issues/RuleDefinitionTest.java`:
- Around line 93-114: Tests in RuleDefinitionTest use the magic number 281 in
multiple places (testDefine, testEmptyExternalFilePath,
testExternalFileWithErrors); extract this into a single static class-level
constant (e.g., EXPECTED_RULE_COUNT or DEFAULT_RULE_COUNT) and replace the
literal 281 in all three test methods with that constant so updates to the
expected built-in rule count are made in one place.
src/test/java/com/github/_1c_syntax/bsl/sonar/ext_issues/RuleDefinitionTest.java
Show resolved
Hide resolved
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Uh oh!
There was an error while loading. Please reload this page.