Добавлена поддержка новых атрибутов качества и важности для диагностик#411
Draft
theshadowco wants to merge 2 commits intodevelopfrom
Draft
Добавлена поддержка новых атрибутов качества и важности для диагностик#411theshadowco wants to merge 2 commits intodevelopfrom
theshadowco wants to merge 2 commits intodevelopfrom
Conversation
WalkthroughРефакторинг класса BSLLanguageServerRuleDefinition: открыты внутренние отображения как публичные константы (REPOSITORY_NAME, RULE_TYPE_MAP, OLD_SEVERITY_MAP, CLEAN_CODE_ATTRIBUTE_MAP, IMPACTS_MAP) и добавлена логика вычисления атрибутов чистого кода и влияний правил по диагностическим тегам; добавлены тесты покрытия для новых карт. Изменения
Оценка трудозатратности рецензирования кода🎯 3 (Умеренно сложно) | ⏱️ ~20 минут Стихотворение
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
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: 1
🧹 Nitpick comments (2)
src/main/java/com/github/_1c_syntax/bsl/sonar/language/BSLLanguageServerRuleDefinition.java (1)
69-73: Публичные мутабельные коллекции — риск непреднамеренного изменения.Карты
OLD_SEVERITY_MAP,RULE_TYPE_MAP,CLEAN_CODE_ATTRIBUTE_MAPиIMPACTS_MAPобъявлены какpublic static final, но самиEnumMap/HashMapостаются мутабельными. Любой внешний код может вызвать.put()/.remove()и повредить маппинг для всего приложения.Рекомендуется обернуть возвращаемые карты в
Collections.unmodifiableMap()в соответствующихcreate*методах.♻️ Пример исправления (для каждого create-метода)
private static Map<DiagnosticSeverity, String> createOldDiagnosticSeverityMap() { Map<DiagnosticSeverity, String> map = new EnumMap<>(DiagnosticSeverity.class); // ... put entries ... - return map; + return Collections.unmodifiableMap(map); }🤖 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 69 - 73, The public static final maps OLD_SEVERITY_MAP, RULE_TYPE_MAP, CLEAN_CODE_ATTRIBUTE_MAP and IMPACTS_MAP are initialized with mutable Map instances exposing the internal state; update each factory method (createOldDiagnosticSeverityMap, createRuleTypeMap, createCleanCodeAttributeMap, createImpactsMap) to wrap the populated map in Collections.unmodifiableMap(...) before returning so external callers cannot mutate the maps and the public constants remain effectively immutable.src/test/java/com/github/_1c_syntax/bsl/sonar/language/BSLLanguageServerRuleDefinitionTest.java (1)
71-81: Хорошие smoke-тесты для полноты маппингов.Тесты корректно проверяют, что все значения
DiagnosticTagпокрыты в обоих маппингах. Однако стоит рассмотреть добавление теста на логику агрегации вcomputeImpact— например, что при нескольких тегах с одинаковымSoftwareQualityвыбирается наивысшийSeverity. Это важная бизнес-логика, не покрытая текущими smoke-тестами.🤖 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/language/BSLLanguageServerRuleDefinitionTest.java` around lines 71 - 81, Add a focused unit test for BSLLanguageServerRuleDefinition.computeImpact that verifies aggregation logic: create multiple DiagnosticTag values that map (via IMPACTS_MAP) to the same SoftwareQuality but different Severity levels, call BSLLanguageServerRuleDefinition.computeImpact(...) with those tags and assert the returned Impact/Severity equals the highest Severity among them; name the test (e.g., smokyComputeImpactAggregation) and use existing enums DiagnosticTag, SoftwareQuality and Severity to locate the relevant mappings and to construct expected results.
🤖 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 154-165: The stream mapping of diagnosticInfo.getTags() to
CLEAN_CODE_ATTRIBUTE_MAP::get and the computeImpact lookup from IMPACTS_MAP may
produce nulls causing NPEs; update the flow to drop missing mappings before
using sorted/findFirst and before accessing impact.getLeft()/getRight(): in the
CLEAN_CODE_ATTRIBUTE_MAP path (diagnosticInfo.getTags() ->
CLEAN_CODE_ATTRIBUTE_MAP::get -> newRule::setCleanCodeAttribute) add a filter to
remove null mappings before distinct/sorted/findFirst, and in computeImpact
(IMPACTS_MAP.get(tag) -> impact.getLeft()/getRight() ->
newRule::addDefaultImpact) skip tags that return null from IMPACTS_MAP or
otherwise handle absent mappings safely so you never call getLeft()/getRight()
on null.
---
Nitpick comments:
In
`@src/main/java/com/github/_1c_syntax/bsl/sonar/language/BSLLanguageServerRuleDefinition.java`:
- Around line 69-73: The public static final maps OLD_SEVERITY_MAP,
RULE_TYPE_MAP, CLEAN_CODE_ATTRIBUTE_MAP and IMPACTS_MAP are initialized with
mutable Map instances exposing the internal state; update each factory method
(createOldDiagnosticSeverityMap, createRuleTypeMap, createCleanCodeAttributeMap,
createImpactsMap) to wrap the populated map in Collections.unmodifiableMap(...)
before returning so external callers cannot mutate the maps and the public
constants remain effectively immutable.
In
`@src/test/java/com/github/_1c_syntax/bsl/sonar/language/BSLLanguageServerRuleDefinitionTest.java`:
- Around line 71-81: Add a focused unit test for
BSLLanguageServerRuleDefinition.computeImpact that verifies aggregation logic:
create multiple DiagnosticTag values that map (via IMPACTS_MAP) to the same
SoftwareQuality but different Severity levels, call
BSLLanguageServerRuleDefinition.computeImpact(...) with those tags and assert
the returned Impact/Severity equals the highest Severity among them; name the
test (e.g., smokyComputeImpactAggregation) and use existing enums DiagnosticTag,
SoftwareQuality and Severity to locate the relevant mappings and to construct
expected results.
src/main/java/com/github/_1c_syntax/bsl/sonar/language/BSLLanguageServerRuleDefinition.java
Show resolved
Hide resolved
|
There was a problem hiding this comment.
🧹 Nitpick comments (2)
src/main/java/com/github/_1c_syntax/bsl/sonar/language/BSLLanguageServerRuleDefinition.java (2)
158-163:.sorted()неявно зависит от порядка объявленияCleanCodeAttributeв SonarQube API.
CleanCodeAttribute— Java enum, и.sorted()без компаратора упорядочивает поordinal(). Выбор итогового атрибута для правила зависит от порядка объявления констант в чужом API, который может поменяться при обновлении версии SonarQube. Комментарий «пока так» это признаёт, но стоит явно зафиксировать намеренный порядок приоритетов или использовать явный компаратор.♻️ Вариант с явным компаратором-заглушкой (для будущего уточнения)
diagnosticInfo.getTags().stream() .map(CLEAN_CODE_ATTRIBUTE_MAP::get) .distinct() - .sorted() + .sorted(Comparator.naturalOrder()) // TODO: заменить на осмысленный приоритет атрибутов .findFirst() .ifPresent(newRule::setCleanCodeAttribute);🤖 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 158 - 163, The current stream pipeline maps diagnosticInfo.getTags() via CLEAN_CODE_ATTRIBUTE_MAP and then uses .sorted() which relies on CleanCodeAttribute's enum ordinal order (unstable across SonarQube versions); instead define and use an explicit priority ordering (e.g., a Comparator<CleanCodeAttribute> or a priority map) and apply it in the stream before findFirst(), so replace the implicit .sorted() with .sorted(yourExplicitComparator) or .min(yourExplicitComparator) when computing the value passed to newRule::setCleanCodeAttribute to make the priority deterministic and independent of the enum declaration order.
212-213:EnumMapвместоHashMapдля ключа-enumSoftwareQuality.Поскольку
SoftwareQuality— перечисление с фиксированным числом значений,EnumMapдаёт чуть лучшую производительность и более явно передаёт намерение.♻️ Предлагаемая замена
- Map<SoftwareQuality, Severity> map = new HashMap<>(); + Map<SoftwareQuality, Severity> map = new EnumMap<>(SoftwareQuality.class);🤖 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 212 - 213, Replace the HashMap with an EnumMap in computeImpact to reflect that keys are the SoftwareQuality enum: change the map declaration/initialization in computeImpact(List<DiagnosticTag> tags) from new HashMap<>() to new EnumMap<>(SoftwareQuality.class) and keep all subsequent uses of the map (put/get/containsKey) unchanged so the method uses EnumMap<SoftwareQuality, Severity> for better performance and clearer intent.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In
`@src/main/java/com/github/_1c_syntax/bsl/sonar/language/BSLLanguageServerRuleDefinition.java`:
- Around line 155-163: The stream pipeline using
diagnosticInfo.getTags().stream().map(CLEAN_CODE_ATTRIBUTE_MAP::get) can produce
nulls from CLEAN_CODE_ATTRIBUTE_MAP::get and later operations (distinct/sorted)
will throw NPE; update the pipeline to filter out null mappings before
distinct/sorted/findFirst and then call newRule.setCleanCodeAttribute only if
present (e.g., map -> filter(Objects::nonNull) or flatMap to Optional),
referencing diagnosticInfo.getTags(), CLEAN_CODE_ATTRIBUTE_MAP::get, and
newRule.setCleanCodeAttribute so the code safely ignores unmapped tags.
- Around line 212-222: computeImpact currently assumes IMPACTS_MAP.get(tag) is
non-null and calls impact.getLeft()/getRight(), which can NPE; change
computeImpact to null-check the result of IMPACTS_MAP.get(tag) (the local
variable impact) and skip the tag if impact is null before accessing
impact.getLeft() or impact.getRight(); ensure the loop over DiagnosticTag still
updates map only when impact is non-null so the method returns a correct
Map<SoftwareQuality, Severity>.
---
Nitpick comments:
In
`@src/main/java/com/github/_1c_syntax/bsl/sonar/language/BSLLanguageServerRuleDefinition.java`:
- Around line 158-163: The current stream pipeline maps diagnosticInfo.getTags()
via CLEAN_CODE_ATTRIBUTE_MAP and then uses .sorted() which relies on
CleanCodeAttribute's enum ordinal order (unstable across SonarQube versions);
instead define and use an explicit priority ordering (e.g., a
Comparator<CleanCodeAttribute> or a priority map) and apply it in the stream
before findFirst(), so replace the implicit .sorted() with
.sorted(yourExplicitComparator) or .min(yourExplicitComparator) when computing
the value passed to newRule::setCleanCodeAttribute to make the priority
deterministic and independent of the enum declaration order.
- Around line 212-213: Replace the HashMap with an EnumMap in computeImpact to
reflect that keys are the SoftwareQuality enum: change the map
declaration/initialization in computeImpact(List<DiagnosticTag> tags) from new
HashMap<>() to new EnumMap<>(SoftwareQuality.class) and keep all subsequent uses
of the map (put/get/containsKey) unchanged so the method uses
EnumMap<SoftwareQuality, Severity> for better performance and clearer intent.
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
1 participant
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.
Правильнее делать атрибуты в диагностика в бсллс.
Этот реквест - просто пример, что надо