Skip to content

Conversation

artbear
Copy link
Contributor

@artbear artbear commented Oct 16, 2021

Описание

  • реализовал правило
  • пока не все тест-кейсы покрыты

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

Closes #536

Чеклист

Общие

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

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

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

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

@artbear
Copy link
Contributor Author

artbear commented Oct 23, 2021

@nixel2007 Предлагаю следующее:

  • ты + мейнтейнеры смотрите этот ПР как есть сейчас
    • не обращая внимания на мелкие юнит-тесты на java
  • я отрабатываю все замечания
  • затем вы\ты сообщаете, что больше замечаний нет
  • я удаляю все мелкие юнит-тесты на java
    • они все равно есть в основном бсл-файле
  • вы\ты мержите ПР, в котором один общий тест и один общий файл
    • т.е. то, что требуется

почему так предлагаю:

  • текущие мелкие юнит-тесты позволяют быстро разобраться в проблеме
  • и быстро доработать правило в случае появления замечаний, ФП, ФН и т.п.
  • поэтому не хочется их удалять, пока работа над правилом не закончена
  • наличие этих тестов позволяет мне быстро вносить правки и доработки

устроит такой вариант?


}

return super.visitFile(file);
Copy link
Member

Choose a reason for hiding this comment

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

@artbear нет смысла спускаться ниже. Просто return file;

@Override
public ParseTree visitFile(BSLParser.FileContext file) {

final var queriesWithParams = documentContext.getQueries().stream()
Copy link
Member

Choose a reason for hiding this comment

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

@artbear если это вынести в отдельный класс, который будет описывать экземпляр Запрос, то диагностика сразу станет легче.

.map(ParameterContext.class::cast)
// .map(ctx -> Pair.of(ctx, "\"" + ctx.name.getText() + "\""))
// если есть несколько одинаковых параметров в запросе
.collect(Collectors.toMap(ctx -> "\"" + ctx.name.getText() + "\"", ctx -> ctx,
Copy link
Member

Choose a reason for hiding this comment

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

@artbear String.format

private Collection<CodeBlockContext> getCodeBlocks() {
final var ast = documentContext.getAst();
var blocks = getSubBlocks(ast);
final var fileCodeBlock = Optional.ofNullable(ast.fileCodeBlock()).map(BSLParser.FileCodeBlockContext::codeBlock);
Copy link
Member

Choose a reason for hiding this comment

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

@artbear зачем разделение на 2 операции? Почему сразу не написать:

Optional.ofNullable(ast.fileCodeBlock()) .map(BSLParser.FileCodeBlockContext::codeBlock) .ifPresent(blocks::add);
var blocks = getSubBlocks(ast);
final var fileCodeBlock = Optional.ofNullable(ast.fileCodeBlock()).map(BSLParser.FileCodeBlockContext::codeBlock);
fileCodeBlock.ifPresent(blocks::add);
final var fileCodeBlockBeforeSub =
Copy link
Member

Choose a reason for hiding this comment

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

@artbear зачем разделение на 2 операции?

.map(BSLParser.AcceptorContext::accessProperty)
.map(BSLParser.AccessPropertyContext::IDENTIFIER)
.map(ParseTree::getText)
.filter(s -> QUERY_TEXT_PATTERN.matcher(s).matches())
Copy link
Member

Choose a reason for hiding this comment

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

@artbear s плохое имя для переменной

private static Optional<ParameterContext> findAppropriateParamFromSetParameter(BSLParser.CallStatementContext callStatementContext,
String queryVarName,
Map<String, ParameterContext> params) {
final var callCtx = Optional.of(callStatementContext);
Copy link
Member

Choose a reason for hiding this comment

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

@artbear Ctx может не стоит использовать сокращение?

}

)
public class MissingQueryParameterDiagnostic extends AbstractVisitorDiagnostic {
Copy link
Member

Choose a reason for hiding this comment

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

@artbear на текущий момент диагностика будет сильно фонить. Причины:

  • Нет извлечения текста запроса из результата выполнения локальной функции
  • Нет очистки (замены) значения экземпляра запроса при новом объявлении
  • Не обработан метод ЗаполнитьЗначенияСвойств(...)
  • Не обработан метод СтрЗаменить(..)
  • Инициализация и установка параметрах могут быть в разных "ветках" кода
  • Не обработано заполнение параметров в другой функции, хотя бы в локальной

private static Optional<ParameterContext> findAppropriateParamFromSetParameterMethod(Optional<BSLParser.CallStatementContext> callCtx,
Map<String, ParameterContext> params) {
return callCtx.map(BSLParser.CallStatementContext::accessCall)
Copy link
Member

Choose a reason for hiding this comment

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

@artbear длинный стрим затрудняет чтение и осознание алгоритма

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

2 participants