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

[FP] RefOveruse #2825

Merged
merged 28 commits into from
Jun 3, 2023
Merged

[FP] RefOveruse #2825

merged 28 commits into from
Jun 3, 2023

Conversation

artbear
Copy link
Contributor

@artbear artbear commented Jun 24, 2022

Описание

новые методы поиска в Trees

  • findAllTopLevelRuleNodes
  • доп.метод findAllRuleNodes с коллекцией

исправление FP

  • исключаю вложенные запросы

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

Closes #2823

Чеклист

Общие

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

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

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

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

@artbear
Copy link
Contributor Author

artbear commented Jun 24, 2022

Точнее, еще одно ФП обнаружилось, когда включил поддержку вложенных запросов.

@artbear
Copy link
Contributor Author

artbear commented Jun 26, 2022

Упала задача javadoc из-за проблем обращения к сайту javadoc. вся остальная сборка прошла успешно.

asosnoviy and others added 2 commits June 29, 2022 16:01
убрал табы из тест-файла для правильного подсчета позиции
@sonarcloud
Copy link

sonarcloud bot commented Jul 1, 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 3 Code Smells

99.3% 99.3% Coverage
0.0% 0.0% Duplication

@nixel2007
Copy link
Member

@artbear поглядишь последний тест?

@artbear
Copy link
Contributor Author

artbear commented Jul 17, 2022

@artbear поглядишь последний тест?

Да, сделаю.

@theshadowco
Copy link
Member

@artbear надо конфликты разрулить

# Conflicts:
#	src/main/java/com/github/_1c_syntax/bsl/languageserver/utils/Trees.java
@artbear
Copy link
Contributor Author

artbear commented Dec 17, 2022

@artbear надо конфликты разрулить

@theshadowco конфликты исправлены

@sonarcloud
Copy link

sonarcloud bot commented Dec 24, 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 20 Code Smells

98.9% 98.9% Coverage
0.0% 0.0% Duplication

import com.github._1c_syntax.bsl.languageserver.utils.Trees;
import com.github._1c_syntax.bsl.parser.BSLParserRuleContext;
import com.github._1c_syntax.bsl.parser.SDBLParser;
import com.github._1c_syntax.bsl.parser.SDBLParser.ColumnContext;
Copy link
Member

Choose a reason for hiding this comment

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

Импорт и SDBLParser и конкретных контекстов

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Исправил, оставил только импорт SDBLParser

Comment on lines 160 to 161
// .filter(tabularSectionTable -> !tabularSectionTable.tableNameOrAlias.isEmpty())// TODO убираешь эти условия, и падает тест со строкой 13
// .filter(tabularSectionTable -> !tabularSectionTable.tabularSectionNames.isEmpty())
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

Choose a reason for hiding this comment

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

Линтер не отключен, этот код просто не был до конца исправлен

исправлено.

.filter(lastChild -> REF_PATTERN.matcher(lastChild.getText()).matches())
.map(BSLParserRuleContext.class::cast);
.filter(columnNode -> columnNode.getChildCount() > COUNT_OF_TABLE_DOT_REF)
// .map(column -> column.getChild(column.getChildCount() - 1))
Copy link
Member

Choose a reason for hiding this comment

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

стоит удалить, отформатировать, позакрывать замечания ide

Copy link
Contributor Author

Choose a reason for hiding this comment

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

код в комментариях удален, форматирование сделано.

* @param indexes - коллекция индексов
* @return найденные узлы
*/
public static Collection<ParserRuleContext> findAllTopLevelRuleNodes(ParserRuleContext t, Collection<Integer> indexes) {
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
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

Choose a reason for hiding this comment

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

Плбс плохое название метода. Оно ищет не все верхнеуровневые узлы, а среди потомков.

исправил название на findAllTopLevelDescendantNodes

также исправил односимвольное имя параметра на root

@nixel2007 @theshadowco


t.children.stream()
.map(node -> findAllTopLevelRuleNodesInner(node, indexes))
.forEachOrdered(result::addAll);
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

Choose a reason for hiding this comment

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

а сортировка зачем?

да, сортировка не нужна. убрал

.hasMessage("0");

}
// @BeforeEach
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

Choose a reason for hiding this comment

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

ну блин! Артур!

упс ) Откатил обратно!

@theshadowco theshadowco self-requested a review March 29, 2023 14:04
var columns = Trees.findAllTopLevelRuleNodes(ctx, RULE_COLUMNS).stream()
.filter(parserRuleContext -> parserRuleContext.getRuleIndex() == SDBLParser.RULE_column)
.filter(parserRuleContext -> Trees.getRootParent((BSLParserRuleContext) parserRuleContext, EXCLUDED_COLUMNS_ROOT)
.getRuleIndex() == SDBLParser.RULE_query)

Check warning

Code scanning / QDJVMC

Nullability and data flow problems

Method invocation 'getRuleIndex' may produce 'NullPointerException'
@ghost
Copy link

ghost commented Mar 31, 2023

👇 Click on the image for a new way to code review

Review these changes using an interactive CodeSee Map

Legend

CodeSee Map legend

@artbear
Copy link
Contributor Author

artbear commented Mar 31, 2023

@nixel2007 @theshadowco все замечания исправлены. тесты зеленые. Есть еще замечания или уже примете ПР? )

@artbear
Copy link
Contributor Author

artbear commented Jun 3, 2023

@nixel2007 @theshadowco все замечания исправлены. тесты зеленые. Есть еще замечания или уже примете ПР? )

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

@nixel2007 nixel2007 merged commit 0f78f59 into 1c-syntax:develop Jun 3, 2023
@nixel2007
Copy link
Member

Спасибо!

@artbear artbear deleted the fix/refOveruse branch June 10, 2023 05:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[FP] RefOveruse
4 participants