Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Опциональный параметр диагностики Typo (caseInsensitive) #2945

Open
wants to merge 3 commits into
base: develop
Choose a base branch
from

Conversation

ovcharenko-di
Copy link
Contributor

Параметр позволяет не учитывать регистр в словаре исключений.

Описание

Добавил необязательный параметр в диагностику Typo, который позволяет не учитывать регистр в словаре исключений.
Реализовано путем приведения элементов коллекции слов-исключений и токенов к нижнему регистру.

Связанные задачи

Closes #2889

Чеклист

Общие

  • Ветка PR обновлена из develop
  • Отладочные, закомментированные и прочие, не имеющие смысла участки кода удалены
  • Изменения покрыты тестами
  • Обязательные действия перед коммитом выполнены (запускал команду gradlew precommit)

Для диагностик

  • Описание диагностики заполнено для обоих языков (присутствуют файлы для обоих языков, для русского заполнено все подробно, перевод на английский можно опустить)

Дополнительно

@nixel2007
Copy link
Member

Не могу не задать вопрос. Что по перфомансу на большой конфигурации типа ерп?


Arrays.stream(camelCaseSplitedWords)
Arrays.stream(camelCaseSplitWords)
.distinct()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Обоснуй?

Copy link
Contributor Author

@ovcharenko-di ovcharenko-di Nov 27, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

в пользовательских конфигах могут использоваться варианты слов с заглавной\строчной, да и случайно могут дубли оказаться.

этот distinct() призван сократить количество элементов коллекции, а даже если дублей нет, то производительность не должна сильно пострадать

if (caseInsensitive) {
camelCaseSplitWords = Arrays.stream(camelCaseSplitWords)
.map(String::toLowerCase)
.toArray(String[]::new);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Если на этом шаге уже есть перегонка в стрим, но нет смысла гонять их стрима в массив и обратно. Можно сразу начать обрабатывать стрим. Предлагаю засунуть в Stream до условия, в условии сделать toLowerCase и distinct() (если он действительно нужен), а вне условия уже оставить оставшийся код стрима.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

стало не актуально

вместо приведения к нижнему регистру я изменил тип коллекции, чтобы можно было их сравнивать либо с учетом, либо без учета регистра в зависимости от параметра диагностики

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Только ты сложность алгоритма увеличил. contains в set работает за O(1), а в List - за O(n)

@nixel2007
Copy link
Member

Спасибо! Пара мелких замечаний.

@ovcharenko-di
Copy link
Contributor Author

Не могу не задать вопрос. Что по перфомансу на большой конфигурации типа ерп?

проверю) нужна будет консультация по тому, как грамотно сделать замер

@nixel2007
Copy link
Member

@ovcharenko-di
https://1c-syntax.github.io/bsl-language-server/contributing/Measures/

Только не забыть выключить защитник винды и прогонять два-три раза подряд (без больших пауз), чтобы на прогретых дисках

@ovcharenko-di
Copy link
Contributor Author

@ovcharenko-di https://1c-syntax.github.io/bsl-language-server/contributing/Measures/

Только не забыть выключить защитник винды и прогонять два-три раза подряд (без больших пауз), чтобы на прогретых дисках

Исправил замечания по Stream->Array->Stream. Проверка ERP без регл. отчетов на одной этой диагностике выполняется с разницей +/- 2 секунды относительно develop.
НО появились FP на всяких аббревиатурах и это проблема. Буду смотреть, в чем дело.

@ovcharenko-di ovcharenko-di changed the title Опциональный параметр диагностики Typo (caseInsensitive) WIP: Опциональный параметр диагностики Typo (caseInsensitive) Nov 30, 2022
@ovcharenko-di
Copy link
Contributor Author

@nixel2007 разобрался. Далее в дело вступает JLanguageTool, который, видимо, пропускает аббревиатуры.

А раз я сейчас все токены лихо привожу к нижнему регистру, то он считает, что это обычное слово. Буду думать, как это исправить.

@EightM , может быть, у тебя есть идеи?

@ovcharenko-di
Copy link
Contributor Author

@ovcharenko-di https://1c-syntax.github.io/bsl-language-server/contributing/Measures/

Только не забыть выключить защитник винды и прогонять два-три раза подряд (без больших пауз), чтобы на прогретых дисках

проверил, разницы особой нет

@ovcharenko-di ovcharenko-di changed the title WIP: Опциональный параметр диагностики Typo (caseInsensitive) Опциональный параметр диагностики Typo (caseInsensitive) Dec 11, 2022
@sonarcloud
Copy link

sonarcloud bot commented Dec 11, 2022

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 2 Code Smells

100.0% 100.0% Coverage
0.0% 0.0% Duplication

@nixel2007
Copy link
Member

@EightM plz review

@nixel2007 nixel2007 requested a review from EightM March 31, 2023 08:21
var delimiter = ",";
String exceptions = SPACES_PATTERN.matcher(info.getResourceString("diagnosticExceptions")).replaceAll("");
if (!userWordsToIgnore.isEmpty()) {
exceptions = exceptions + delimiter + SPACES_PATTERN.matcher(userWordsToIgnore).replaceAll("");
}

if (caseInsensitive) {
exceptions = exceptions.toLowerCase();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

В параметрах lowerCase должна быть явно указана локаль (в проекте вроде используется English)
https://rules.sonarsource.com/java/RSPEC-1449


@Override
public void configure(Map<String, Object> configuration) {
super.configure(configuration);
minWordLength = Math.max(minWordLength, DEFAULT_MIN_WORD_LENGTH);
}

private Set<String> getWordsToIgnore() {
private List<String> getWordsToIgnore() {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

В чем логика замены Set на List?

.filter(Predicate.not(String::isBlank))
.filter(element -> element.length() >= minWordLength)
.filter(Predicate.not(wordsToIgnore::contains))
.filter(element -> wordsToIgnore.stream().noneMatch(word
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Здесь замена Set на List сделала поиск слова медленнее. contains в сете работает за О(1), здесь же теперь линейный поиск за O(N), в результате сложность всего стрима стала из линейной квадратичной

@theshadowco
Copy link
Member

@ovcharenko-di
Добьем?

@ovcharenko-di
Copy link
Contributor Author

@theshadowco на своих проектах я как-то уже смирился с тем, что словарь чувствителен к регистру

сейчас я бы вообще закрыл этот issue, но если сообщество считает, что надо реализовать - могу добить, в принципе)
может, устроим голосование в чате?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants