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

Директивы компиляции и аннотации в MethodSymbol #1198

Merged
merged 19 commits into from
Jun 9, 2020

Conversation

artbear
Copy link
Contributor

@artbear artbear commented May 16, 2020

Описание

директивы компиляции и аннотации методов в MethodSymbol

  • устранены замечания
  • bsl-код из мелких юнит-тестов скопирован в общий bsl-файл и общий тест
  • доработано использование директив компиляции
    • &НаКлиентеНаСервереБезКонтекста
    • &НаКлиентеНаСервере
  • есть поддержка аннотаций OneScript

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

Closes:#1115

Чеклист

Общие

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

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

@otymko
Copy link
Member

otymko commented May 16, 2020

@artbear приведи плиз к общему виду тесты в MethodSymbolComputerTest. Не используй:
image
У тебя есть общий файл MethodSymbolComputerTest.bsl, используй код из него.

@otymko otymko self-requested a review May 16, 2020 16:14
Copy link
Member

@otymko otymko left a comment

Choose a reason for hiding this comment

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

Нужно привести тесты к общему виду.

.map(annotation -> annotation.getStop().getType())
.map(Annotation::of)
.map(optionalAnnotation -> optionalAnnotation.orElse(null))
.filter(Objects::nonNull)
Copy link
Member

@nixel2007 nixel2007 May 18, 2020

Choose a reason for hiding this comment

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

лучше проверить на isPresent/isEmpty. тогда предыдущий шаг не нужен

Copy link
Contributor Author

Choose a reason for hiding this comment

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

сделано

import java.util.Optional;
import java.util.stream.Stream;

public enum Annotation {
Copy link
Member

@nixel2007 nixel2007 May 18, 2020

Choose a reason for hiding this comment

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

для аннотаций было бы еще полезно добавить информацию об имени аннотации (важно для custom) и списке ее параметров. т.е. превратить Annotation в AnnotationKind (как ты изначально предлагал в апи), и добавить дополнительный класс (возможно даже символ) Annotation.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

сделано.

  • добавлена информация об имени аннотации и списке параметров
  • выделены отдельные классы Annotation, AnnotationKind и AnnotationParameterDefinition

import java.util.Optional;
import java.util.stream.Stream;

public enum CompilerDirective {
Copy link
Member

Choose a reason for hiding this comment

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

CompilerDirectiveKind

Copy link
Contributor Author

Choose a reason for hiding this comment

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

переименовал

//&НаСервереБезКонтекста
//&НаКлиентеНаСервереБезКонтекста
//аналогично
//т.е. порядок этих 2х директив не важен, все равно используется &НаКлиентеНаСервереБезКонтекста.
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.

@nixel2007 на 3х директивах проверял, падает с ошибками, совместимы только те, что в комментариях.

или ты про 3 и более, когда одинаковые директивы есть?

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.

Проверил одинаковые директивы.

  • Если полный дубль, например, пару &НаКлиенте и &НаКлиентеНаСервере продублировать 2 или более раз, ошибок не выдается.

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

  • для пар &НаСервереБезКонтекста и &МояНаСервереБезКонтекста поведение аналогично

т.е. все остается также и текущая реализация работает.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

тест для проверки также сделал

@artbear
Copy link
Contributor Author

artbear commented May 31, 2020

все тесты привел к общему виду.
@otymko @nixel2007 @theshadowco посмотрите.

@nixel2007
Copy link
Member

Спасибо!

@nixel2007 nixel2007 merged commit ef3c961 into 1c-syntax:develop Jun 9, 2020
@artbear artbear deleted the annotations-method-symbol-1115 branch June 9, 2020 09:40
@artbear
Copy link
Contributor Author

artbear commented Jun 9, 2020

УРА, и этот долгострой приняли.

теперь можно наконец-то делать правило, ради которого этот ПР создавался )

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.

4 participants