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

Все параметры запроса инициализированы #1882

Draft
wants to merge 9 commits into
base: develop
Choose a base branch
from

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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Все параметры запроса инициализированы
2 participants