-
Notifications
You must be signed in to change notification settings - Fork 72
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
Add a command line interface #237
Conversation
С CMakeLists я не особо понял. Если надо добавить, то разберусь |
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.
Не очень нравится разбиение на файлы и то, как они называются. dictionaries
конечно действительно хранить dictionaries, а functions
хранит functions
, но это слишком общие названия и выглядит так, будто мы группируем функциональность не по её логическому предназначению, а по тому, чем конкретные сущности в коде с точки зрения языка являются. Я бы пытался разбивать именно логически, если такое разбиение вообще возможно. Если нет, то в целом это и одним файлом можно оставить
cli/dictionaries.py
Outdated
'pyro': desbordante.Pyro(), | ||
'tane': desbordante.Tane(), | ||
'hyfd': desbordante.HyFD(), | ||
'fd_mine': desbordante.FdMine(), | ||
'dfd': desbordante.DFD(), | ||
'dep_miner': desbordante.Depminer(), | ||
'fdep': desbordante.FDep(), | ||
'fun': desbordante.FUN(), | ||
'fastfds': desbordante.FastFDs(), | ||
'aid': desbordante.Aid(), | ||
'naive_fd_verifier': desbordante.FDVerifier(), | ||
'naive_afd_verifier': desbordante.FDVerifier(), | ||
'naive_mfd_verifier': desbordante.MetricVerifier() |
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.
Вообще у нас есть похожий енам в c++, делающий сопоставление, как будто можно попытаться переиспользовать, но я на самом деле не уверен, что нужно
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.
не стал с этим ничего делать
cli/desbordante.py
Outdated
@@ -0,0 +1,43 @@ | |||
#!/usr/bin/python3 | |||
|
|||
import click |
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.
- Внешнюю зависимость необходимо указать в requirements.txt
https://pip.pypa.io/en/stable/cli/pip_freeze/
Вот тут подробнее зачем:
https://www.askpython.com/python/python-requirements-txt - Нужно указать в readme.md, что необходимо ставить дополнительные пакеты через requirements.txt
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.
readme думаю можно пока не менять, потому что оно совсем скоро обновится, там сразу и опишем процесс сборки. requirements.txt нужны, да
cli/functions.py
Outdated
match algo: | ||
case 'naive_fd_verifier': | ||
result = dictionaries.ALGOS[algo].fd_holds() | ||
case 'naive_afd_verifier': | ||
result = dictionaries.ALGOS[algo].get_error() | ||
case 'naive_mfd_verifier': | ||
result = dictionaries.ALGOS[algo].mfd_holds() | ||
case _: | ||
result = dictionaries.ALGOS[algo].get_fds() |
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.
Нужно в readme.md ещё указать версию питона, видимо, как минимум 3.10
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.
с использованием StrEnum стало 3.11
cli/functions.py
Outdated
def fill_remaining_opts(args): | ||
for name in dictionaries.OPTS.keys(): | ||
if not value_is_none(args[name]): | ||
dictionaries.REMAINING_OPTS.append(name) | ||
|
||
|
||
def fill_opts(): | ||
for algo in dictionaries.ALGOS.values(): | ||
for opt in algo.get_possible_options(): | ||
dictionaries.OPTS.update({opt: algo.get_option_type(opt)}) |
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.
Вот тут я задумался над тем, можно ли сделать какой-то класс для dictionaries и эти функции как методы отправить туда? Тут я не уверен, но может улучшить код в целом
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.
Кажется, что дописать поддержку AR теперь будет настолько просто, что это можно сделать прямо здесь.
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.
Ещё вынести все переиспользуемые строковые литералы в константы (ERROR = 'error'
) в начале файла и опциям передавать f'--{ERROR}'
.
Если опять чего-то не пропущу, то это должно быть последнее ревью.
P.S. коммиты ещё надо в порядок привести, слей их все в один большой
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.
Было бы ещё хорошо расставить в функциях хинты с типами, если можешь. Чтобы, допустим, можно было прогнать mypy потом
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.
Ладно, не последний. Тут ещё помимо отмеченного куча всякой мелочи, но мне не хочется сильно задерживать релиз
Тайп хинты можно ещё поправить потом или сейчас (например, можно писать str | None
, dict[KeyType, ValueType]
, desbordante.Algorithm
)
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.
Пойдёт
Add command line interface for fd/fd verification, afd/afd verification, mfd verification
Add a command line interface with options: task, algorithm, table, algorithm parameters and some others. It can find fds, afds, verify fds, afds or mfds and print the result to the console or to a file.