-
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
Границы обращения к метаданным #1837
base: develop
Are you sure you want to change the base?
Conversation
# Conflicts: # docs/diagnostics/index.md # docs/en/diagnostics/index.md
# Conflicts: # docs/diagnostics/index.md # docs/en/diagnostics/index.md
Обновление из головного репо
...ain/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/MetadataBordersDiagnostic.java
Outdated
Show resolved
Hide resolved
...ain/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/MetadataBordersDiagnostic.java
Show resolved
Hide resolved
...ain/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/MetadataBordersDiagnostic.java
Outdated
Show resolved
Hide resolved
...ain/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/MetadataBordersDiagnostic.java
Outdated
Show resolved
Hide resolved
...ain/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/MetadataBordersDiagnostic.java
Outdated
Show resolved
Hide resolved
...ain/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/MetadataBordersDiagnostic.java
Outdated
Show resolved
Hide resolved
...ain/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/MetadataBordersDiagnostic.java
Outdated
Show resolved
Hide resolved
|
||
for (Map.Entry<String, String> entry: metadataBordersParameters.entrySet()) { | ||
|
||
if (entry.getKey().isBlank() || entry.getValue().isBlank()) { |
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.
это условие нужно проверять на этапе configure
и создавать мапу без неверных значений
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.
есть замечания
...ain/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/MetadataBordersDiagnostic.java
Outdated
Show resolved
Hide resolved
поправил замечания |
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.
и еще немного предложений по оптимизации
...com/github/_1c_syntax/bsl/languageserver/diagnostics/MetadataBordersDiagnostic_ru.properties
Outdated
Show resolved
Hide resolved
...com/github/_1c_syntax/bsl/languageserver/diagnostics/MetadataBordersDiagnostic_en.properties
Outdated
Show resolved
Hide resolved
...ain/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/MetadataBordersDiagnostic.java
Outdated
Show resolved
Hide resolved
заработало через visitFile |
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.
Есть вопросы
...main/resources/com/github/_1c_syntax/bsl/languageserver/configuration/parameters-schema.json
Outdated
Show resolved
Hide resolved
docs/diagnostics/MetadataBorders.md
Outdated
|
||
| Имя | Тип | Описание | Значение<br>по умолчанию | | ||
|:---------------------------:|:--------:|:------------------------------------------------------------------------------------------------------------:|:------------------------------:| | ||
| `metadataBordersParameters` | `Строка` | `JSON-структура для пар "регулярное выражение для операторов":"регулярное выражение для имени файла модуля"` | `` | |
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.
Где-то в ранних версиях здесь был пример заполнения. Кажется, стоит его вернуть. Плюс добавить {}
...ain/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/MetadataBordersDiagnostic.java
Outdated
Show resolved
Hide resolved
private List<Pattern> statementPatterns = Collections.emptyList(); | ||
|
||
private static Map<Pattern, Pattern> MapFromJSON(String userSettings) { | ||
ObjectMapper mapper = new ObjectMapper(); |
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.
создание ObjectMapper на каждый configure - довольно дорогая операция. лучше его закэшировать или просто указать его конструкторе - его туда подложит спринг.
|
||
private static Map<Pattern, Pattern> MapFromJSON(String userSettings) { | ||
ObjectMapper mapper = new ObjectMapper(); | ||
MapType mapType = mapper.getTypeFactory().constructMapType(HashMap.class, String.class, String.class); |
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.
mapType тоже можно закэшировать в конструторе по принятому инстансу обджект-маппера
...ain/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/MetadataBordersDiagnostic.java
Outdated
Show resolved
Hide resolved
...ain/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/MetadataBordersDiagnostic.java
Outdated
Show resolved
Hide resolved
Map<String, String> stringMap = mapper.readValue(userSettings, mapType); | ||
return stringMap.entrySet().stream() | ||
.filter(entry -> ! entry.getKey().isBlank() && ! entry.getValue().isBlank()) | ||
.collect(Collectors.toMap( |
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.
пробовал делать включать замер производительности? диагностика не вырывается в топ?
} | ||
}); | ||
|
||
return super.visitStatement(ctx); |
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.
если ты проверяешь текст statement целиком, то нет смысла дергать super, т.к. вложенные стэйтменты проверят тот же самый кусок текста. т.е. здесь будет фонить на вложенных конструкциях вида
Если Истина Тогда
Если Истина Тогда
ЗапрещенныйВызов(); // тут кинет два срабатывания на одно и то же место
КонецЕсли;
КонецЕсли;
...main/resources/com/github/_1c_syntax/bsl/languageserver/configuration/parameters-schema.json
Outdated
Show resolved
Hide resolved
@@ -0,0 +1,3 @@ | |||
diagnosticMessage=Обращение к метаданным за пределами разрешенных границ |
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.
может быть в сообщение стоит добавить регулярку/пару регулярок, на которой произошло срабатывание?
| `metadataBordersParameters` | `Строка` | `JSON-структура для пар "регулярное выражение для операторов":"регулярное выражение для имени файла модуля"` | `` | | ||
| Имя | Тип | Описание | Значение<br>по умолчанию | | ||
|:---------------------------:|:--------:|:---------------------------------------------------------------------------------------:|:------------------------------:| | ||
| `metadataBordersParameters` | `Строка` | `{"регулярное выражение для операторов":"регулярное выражение для имени файла модуля"}` | `` | |
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.
стало еще менее понятно :(
Предлагаю что-то вроде:
Строка в виде объекта JSON (в фигурных скобках) с перечислением пар "регулярное выражение для операторов": "регулярное выражение для имени файла модуля". Например, `{"Регистры?Сведений.КонтактнаяИнформация": "CommonModules/РаботаСКонтактами"}`.
Пускай пример будет прям в описании параметра, упростит понимание.
@Override | ||
public ParseTree visitFile(BSLParser.FileContext ctx) { | ||
statementPatterns = metadataBordersParameters.entrySet().stream() | ||
.filter(entry -> ! entry.getValue().matcher(documentContext.getUri().getPath()).find()) |
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.
ну и стоит проверять заполненность statementPatterns: для пустой делать возврат и не греть воздух
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?
вообще у нее scope захватывает OS, а для оскрипта mdoRef сейчас выглядит в виде uri. с другой стороны, вряд ли ее кто-то будет включать на os
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.
ну и стоит проверять заполненность statementPatterns: для пустой делать возврат и не греть воздух
это сделано при формировании мапы паттернов. из стринговой мапы в мапу щаблонов попадают только те элементы, в которых и ключ, и значение не пустые
|
||
statementPatterns.forEach(pattern -> { | ||
Matcher matcher = pattern.matcher(ctx.getText()); | ||
while (matcher.find()) { |
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.
потому что матчер возвращает срабатывание по одному за раз
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.
100 раз "addDiagnostic(ctx)"
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.
так, стоп, тут же где-то был другой код, когда в addDiagnostic передается start() и end() от матчера... или это в соседней диагностике?)
@Golovanoff @nixel2007 |
был артуррилион замечаний, я поправил, пришел Никита и спросил "а нафига так-то? ". на этом моменте я сломался 😀 |
Описание
Диагностика позволяет найти обращение к метаданным (через поиск регулярным выражением) за пределами указанных границ. Например, вне какого-то определенного общего модуля.
Связанные задачи
Closes
Чеклист
Общие
gradlew precommit
)Для диагностик
Дополнительно