-
Notifications
You must be signed in to change notification settings - Fork 109
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
Доработка ReferenceIndex - Исправлен расчет ссылок на символы-методы + падение анализа #2833
Conversation
- когда имя параметра совпадает с именем общего модуля - слегка уточнил тест
@@ -46,6 +50,8 @@ class ReferenceIndexTest { | |||
|
|||
@Autowired | |||
private ReferenceIndex referenceIndex; | |||
@Autowired | |||
private LocationRepository locationRepository; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Не понимаю, зачем здесь этот импорт. И не хотелось бы здесь его иметь.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
другого способа сделать проверку поведения я не нашел.
пробовал через referenceIndex получать информацию, но не удалось сделать падающий тест.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@nixel2007 понял, почему у меня не получалось.
в тесте я указывал ссылки на несуществующие методы обшего модуля, поэтому и не получилось.
указал существующие, упал тест, в котором достаточно refIndex
сейчас закоммичу пару коммитов - 1й падающий тест, чтобы была видна разница, и 2й - исправление падения
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
с падающим тестом "неудача" ))
- ведь исправление падения уже закоммичено.
в общем, теперь все верно - использую только публичный АПИ refIndex
обращение к существующим методам общего модуля
МР готов, обе проблемы исправлены. |
Только Сонар что-то обозлился ( |
@nixel2007 посмотри, плиз. жду мержа ) |
continue; | ||
} | ||
addMethodCall(mdoRef, moduleType, methodNameText, Ranges.create(methodName)); | ||
} | ||
} | ||
|
||
private boolean isParamNameEqualCommonModule(String mdoRef, Configuration configuration) { | ||
return subParameters.stream() | ||
.map(configuration::getCommonModule) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Думаю, можно выиграть немного вынеся преобразование имен в общие модули в инициализацию subParameters
. И тогда конфигурацию можно будет убрать из параметров
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
согласен.
в коллекции теперь mdoRef
private Collection<String> calcParams(@Nullable BSLParser.ParamListContext paramList) {
if (paramList == null) {
return Collections.emptySet();
}
final var configuration = documentContext.getServerContext().getConfiguration();
return paramList.param().stream()
.map(BSLParser.ParamContext::IDENTIFIER)
.filter(Objects::nonNull)
.map(ParseTree::getText)
.map(configuration::getCommonModule)
.filter(Optional::isPresent)
.flatMap(Optional::stream)
.map(mdCommonModule -> mdCommonModule.getMdoReference().getMdoRef())
.collect(Collectors.toSet());
}
assertThat(referencesTo).hasSize(3); | ||
} | ||
|
||
// TODO еще нужен тест для параметра, совпадающего с именем общего модуля, ссылки на общий модуль при этом не должны получаться |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ниже разве тест не это проверяет?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
устаревший коммент. удаляю.
if (!DEFAULT_MODULE_TYPES.contains(moduleType)) { | ||
var moduleType = e.getKey(); | ||
if (!DEFAULT_MODULE_TYPES.contains(moduleType) | ||
|| (moduleType == ModuleType.CommonModule && isParamNameEqualCommonModule(mdoRef, configuration))) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Сравнение имени общего модуля с именами параметров кажется более эффективным.
В этом цикле у нс есть uri в значении итератора. По нему можно их коллекции configuration.modulesByObject()
получить описание общего модуля с именем. И это имя сравнить с subParameters
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
тут ИМХО лучше сделать по-другому, не вычисляю uri, а использую уже переданный mdoRef общего модуля
private boolean isParamNameEqualCommonModule(String mdoRef) {
return сommonModuleMdoRefFromSubParams.stream()
.anyMatch(commonModuleMdoRef -> commonModuleMdoRef.equals(mdoRef));
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Даже еще лучше
тип commonModuleMdoRefFromSubParams
будет Set
и эта проверка будет выглядеть так (moduleType == ModuleType.CommonModule && commonModuleMdoRefFromSubParams.contains(mdoRef))
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Лучше или хуже тут мерить, нужно.
6bf9fb3
to
c262312
Compare
# Conflicts: # src/main/java/com/github/_1c_syntax/bsl/languageserver/references/ReferenceIndexFiller.java
This reverts commit 6136f67.
Упал один этап сборки, глюк ГА. |
src/main/java/com/github/_1c_syntax/bsl/languageserver/references/ReferenceIndexFiller.java
Outdated
Show resolved
Hide resolved
src/main/java/com/github/_1c_syntax/bsl/languageserver/references/ReferenceIndexFiller.java
Outdated
Show resolved
Hide resolved
if (!DEFAULT_MODULE_TYPES.contains(moduleType)) { | ||
final var configuration = documentContext.getServerContext().getConfiguration(); | ||
Map<ModuleType, URI> modules = configuration.getModulesByMDORef(mdoRef); | ||
for (Map.Entry<ModuleType, URI> e : modules.entrySet()) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ранее в этом месте перешли к обходу только ключей, чтоб уйти от лишнего
var moduleType = e.getKey();
А сейчас затираем эти изменения.
Нужно изменить.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
исправлено.
кстати, с новым mdclasses в девелопе теперь есть двуязычные mdoRef. не помню, тут это надо было или нет. |
…ces/ReferenceIndexFiller.java Co-authored-by: Nikita Fedkin <[email protected]>
…ces/ReferenceIndexFiller.java Co-authored-by: Nikita Fedkin <[email protected]>
улучшена проверка ЭтотОбъект через регулярку
@@ -172,6 +209,9 @@ private void addMethodCall(String mdoRef, ModuleType moduleType, String methodNa | |||
} | |||
|
|||
private void addCallbackMethodCall(BSLParser.CallParamContext methodName, String mdoRef) { | |||
if (mdoRef.isEmpty()){ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
а в каком случае сюда может прилететь пустой mdoRef?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
а в каком случае сюда может прилететь пустой mdoRef?
в этот параметр передается результат getModule, который теперь может возвращать пустую строку, если не может получить модуль.
- важно, чтобы случайно не обработать неподдерживаемую ссылку виды ОбщийМодуль("Имя") или ОбщегоНазначения.ОбщийМодуль("Имя")
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
мне самому не нравится проверка mdoRef на пустоту.
но другой вариант с выносом этой проверки на уровень выше также не очень - дубль проверки на пустоту будет.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Не совсем понимаю. Где? В какой строке? Я не могу припомнить ситуации, чтобы мдореф был пуст. Если MdoRefBuilder не может построить mdoRef, он возвращает uri
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
посмотри код вызова вызовов getModule и сам метод getModule и увидишь.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Посмотрел. мне не нравится, что у метода addCallbackMethodCall появилось право не добавлять вызов метода. лучше все же продублировать проверку на пустоту, но не размазывать ответственность. Либо сделать, чтобы getModule возвращал Optional, например.
src/main/java/com/github/_1c_syntax/bsl/languageserver/references/ReferenceIndexFiller.java
Show resolved
Hide resolved
…ces/ReferenceIndexFiller.java
Kudos, SonarCloud Quality Gate passed! |
Описание
ловится вызов
ПервыйОбщийМодуль.ЕщеОдинМетодНесуществующий().Реквизит = 10;
если имя параметра метода совпадает с именем общего модуля, то в методе не должны быть найдены ссылки на общий модуль с таким именем
Исправлено падение анализа в Доработка ReferenceIndex - Исправлен расчет ссылок на символы-методы + падение анализа #2833 (comment)
Связанные задачи
Closes #2832
Чеклист
Общие
gradlew precommit
)Для диагностик
Дополнительно