Docstrings + global refactoring issues #45
Conversation
| @@ -97,10 +114,19 @@ def __add__(self, item: 'AbstractToken') -> 'AbstractToken': | |||
| container_token.tokens.extend(container_token.filter_tokens(nested_tokens)) | |||
| return container_token | |||
|
|
|||
| # TODO: достаточно было бы дёргать за is_cancelled, которое нужно сделать геттером. | |||
There was a problem hiding this comment.
Вот тут описал, как я это примерно вижу.
Если в наследниках тоже всё попрятать как минимум в protected, то на выходе остаются 6 публичных объектов:
- is_cancelled - property (getter)
- cancelled()
- keep_on()
- cancel()
- check()
- wait()
Всё. Чистота и порядок.
There was a problem hiding this comment.
Если совсем честно, то наличие cancelled() и cancel() может легко ввести в заблуждение, и одно неверное движение - и привет токен. Имхо, публичный cancelled() - лишний, лучше его спрятать. От токета мы ждём всё-таки статус, а не пытаемся с ним играть в процессе.
По-хорошему, keep_on и check тоже должны быть свойствами, т.к. ничего не принимают в себя и выдают либо статус, либо ошибку (хотя, может check это и не касается).
There was a problem hiding this comment.
У check() есть опциональные аргументы, его точно не сделать свойством. По остальным вроле план хороший, но я еще чекну разок, нет ли чего-то еще публичного.
There was a problem hiding this comment.
Есть. В каждом классе как минимум их атрибуты. Мы же не собираемся на лету подменять набор токенов или тайм-аут или что там ещё...
| # TODO: Как я понимаю, это ещё одна обёртка над логикой is_cancelled. | ||
| # И, судя по всему, дальше она должна использоваться только под капотом. | ||
| # В этом случае было бы проще иметь is_cancelled как геттер и в остальном коде | ||
| # спрашивать not self.is_cancelled вместо вызовов keep_on(). |
There was a problem hiding this comment.
Смотри, суть текущего API в следующем:
- Есть люди, которые предпочитают проперти, и есть люди, которые предпочитают "явное неявному", т.е. вызов методов. Я решил сделать и то и другое, чтобы не выбирать, в какой лагерь становиться. Это почему существуют одновременно
cancelledиis_cancelled(). - По аналогии, кому-то удобно проверять позитивное условие (что токен действителен), кому-то негативное (что он отменен). Отсюда наличие
keep_on(), это просто инвертированная версияis_cancelled(). - Кто-то еще может предпочесть явным проверкам кидание исключения, для таких существует
check().
Крч везде, где я предполагал возможными бессмысленные споры такого рода, я просто реализовывал обе возможности и давал выбор пользователю.
There was a problem hiding this comment.
Как по мне, странное решение, но ты автор, ты так видишь =)
| token = TimeoutToken(timeout) | ||
|
|
||
| return WaitCoroutineWrapper(step, self + token, token) | ||
| # TODO: А зачем этот метод что-то возвращает? Ну отменили и отменили. |
There was a problem hiding this comment.
Это делается для возможности конструкций вида:
SimpleToken().cancel().check()There was a problem hiding this comment.
Во-первых, это неочевидно ни разу.
Во-вторых, DefaultToken так не умеет. Я понимаю, что и не должен, но если у нас есть код, который перебирает какую-то пачку токенов и делает вот такое, то это гарантированный выстрел в колено.
Имхо, все наследники должны вести себя одинаково.
|
|
||
| # TODO: видимо, это внутренний метод и не нужен конечному пользователю. |
There was a problem hiding this comment.
Да, это внутренний метод, в документации его нет.
|
|
||
| # TODO: видимо, это внутренний метод и не нужен конечному пользователю. | ||
| # Плюс, меня постоянно смущает аргумент direct. Никак не могу понять, что он обозначает и почему так назван. |
There was a problem hiding this comment.
Это протекшая выше по иерархии наследования часть функциональности CounterToken. Некоторые обращения к счетчику не должны его декрементить.
| :param cancelled: boolean flag indicating whether this token is cancelled, by default ``False`` | ||
| :param suppress_exceptions: boolean flag indicating whether exceptions should be suppressed, by default ``True`` | ||
| :param default: ???? | ||
| :param before: ???? |
There was a problem hiding this comment.
Это коллбек, который дергается до проверки условия. Позволяет зашивать некоторую логику подготовки для вызова функции-условия. Полезная штука, если пользоваться только лямбдами.
Скажем, для тестов веб-страниц на селениуме с помощью этих коллбеков можно делать примерно такие конструкции (могу ошибиться конкретно в синтаксисе селениума, но думаю суть будет понятна):
token = TimeoutToken(5) + ConditionToken(lambda: page.is_here_some_button(), before=lambda: page.reload())
token.wait()
# Обновлять страницу в течение 5 секунд и проверять, появилась ли там нужная нам кнопка. Если появилась - выйти из цикла. В данном случае before используется для обновления страницы.| :param suppress_exceptions: boolean flag indicating whether exceptions should be suppressed, by default ``True`` | ||
| :param default: ???? | ||
| :param before: ???? | ||
| :param after: ???? |
| :param caching: boolean flag indicating whether to use caching or not, by default ``True`` ????????? | ||
| """ | ||
|
|
||
| # TODO: лучше protected |
There was a problem hiding this comment.
В гисте показал, как я это вижу.
| exception = ConditionCancellationError | ||
|
|
||
| def __init__(self, function: Callable[[], bool], *tokens: AbstractToken, cancelled: bool = False, suppress_exceptions: bool = True, default: bool = False, before: Callable[[], Any] = lambda: None, after: Callable[[], Any] = lambda: None, caching: bool = True): | ||
| def __init__( |
There was a problem hiding this comment.
Стилистические правки, думаю, тут неуместны.
There was a problem hiding this comment.
Это я делал для себя, чтобы разобраться в сигнатуре. Но мы же оба за читабельность, да? )
There was a problem hiding this comment.
Читаемость понятие растяжимое, думаю сейчас это вне скоупа.
| @@ -46,42 +57,48 @@ def __repr__(self) -> str: | |||
| return f'{type(self).__name__}({glued_chunks})' | |||
|
|
|||
| def __str__(self) -> str: | |||
| """Print out the token current status.""" | |||
| # TODO: это единственное место, где в is_cancelled передаётся direct. Этот параметр там точно нужен? | |||
There was a problem hiding this comment.
Да. Суть в том, чтобы каунтертокен при распечатывании не декрементился.
There was a problem hiding this comment.
Опять же, в гисте показал, как я это вижу.
Привет! Муза немножко снизошла. )
Чтобы было проще, предлагаю стащить эту ветку на локальную машину, открыть её в любимой IDE и попробовать в чистом файле импортнуть любой токен from cantok и посмотреть, что выпадает при наведении мышки на класс или на конструктор.
После этого лучше всего посмореть в монитор
TODO(в пайчарме во всяком случае он есть) и идти по этим комментариям.