-
Notifications
You must be signed in to change notification settings - Fork 4
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
chore(refactoring): split to class-based version #22
Conversation
Из предложений по улучшению:
|
Спасибо большое, посмотрю на следующей неделе. Как раз в планах было отрефакторить |
Сделал еще кусок рефакторинга:
Вроде ничего не успустил, думаю к выходным добью рефакторинг... Еще думаю на счет миграции уже существующих данных пользователей, пока в голову приходит какой-то скрипт, который перетащит данные в нужные места, однако не уверен, что это хорошая затея. @llirrikk что думаешь об этом? Сколько вообще пользователей в продакшене? Насколько сложно будет руками перенести? |
С SQLAlchemy и Alembic не работал, но вот, кажется, пришло их время. Они не могут как django migrations сделать автоматически новую таблицу из существующей в БД?
нужные места - это новая таблица в БД? Чем она отличается от старой? Только названиями таблиц и колонок? На данный момент у нас 665 пользователей, из которых 520 активных, поэтому если руками (скриптом), то придется останавливать продакшн на время этих миграций, чтобы пользователи не могли вносить изменения в это время |
Там выходит такая история, отталкиваясь от SQL файлов я написал модели app/models/admin/AdminStatistics.py, app/models/users/UserStatus.py, app/models/users/UserNotifySettings.py и выполнил команду, после чего Alembic сгенерировал файл миграции в котором происходит создание этих моделей - app/migrations/version/e09d97658b84_initial_migration.py. В дальшейнем, если нужно внести какие-то изменения в БД, то они делаются именно в моделях и после чего запуском скрипта генерируется миграция, которая так-же скриптом применяется. А вот с дата миграциями все немного сложнее, это придется делать скриптом, тк дата тайпы отличаются и SQLAlchemy не сможет работать с прошлым файлом базы. Я больше переживаю о файлах пользователей, в которых хранятся данные, я планирую их переместить в другое месте, конкретнее app/storage или просто storage. В целом заменить все методы работы с файлами на централизованные, по этому нужно будет потом подумать, как корректно мигрировать именно userdata. Давай созвонимся сегодня в telegram |
Команда для создания новой миграции с автоматической проверкой изменений:
Команда что бы применить изменения:
|
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.
👍, но pep8 не везде соблюден
from sqlalchemy import create_engine | ||
from sqlalchemy.orm import scoped_session, sessionmaker | ||
|
||
engine = create_engine(config.DATABASE_URL, convert_unicode=True) | ||
return scoped_session(sessionmaker(autocommit=False, autoflush=False, bind=engine)) |
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.
может логику инициализации БД вынести в db/__init__.py
?
def insert_data(self) -> bool: | ||
if not self.need_to_add_values(): | ||
return False | ||
|
||
values = self.values() if callable(self.values) else self.values | ||
|
||
for value in values: | ||
model = self.model() | ||
getattr(model, self.fill_method_name)(**value) | ||
model.save() |
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.
Возвращаемый bool
нигде далее не обрабатывается. Возможно, надо заменить на None
, ну или raise что-то. К тому же из функции может вовсе ничего не возвращаться, что противоречит аннотации
if text is not None: | ||
dispatcher_.register_message_handler(handler_class.process, text=text, state=state) | ||
|
||
if commands is not None: | ||
dispatcher_.register_message_handler(handler_class.process, commands=commands, state=state) | ||
|
||
if text is None and commands is None and state is not None: | ||
dispatcher_.register_message_handler(handler_class.process, state=state) |
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.
В 3.10 появился match/case, может, его сюда засунуть?
|
||
import db.admins_statistics | ||
from app.handlers.commands.settings import NotificationSettingsCommandHandler | ||
from config import Config |
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.
Почему тут Config
, если везде config
?
from app.helpers import OrioksHelper, TelegramMessageHelper | ||
from app.menus.orioks import OrioksAuthFailedMenu | ||
from app.menus.start import StartMenu | ||
from config import Config |
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.
То же самое?
|
||
__tablename__ = 'user_status' | ||
|
||
user_telegram_id = Column(Integer, nullable=False) |
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.
Наверное лучше прописать связь с таблицей user_notify_settings
как One To One
class BaseModel(DeclarativeModelBase): | ||
__abstract__ = True | ||
|
||
id = Column(Integer, primary_key=True) |
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.
Почему pk именно на id? Думаю лучше ставить на user_telegram_id
ну и убрать id
created_at = Column(DateTime, default=func.now()) | ||
updated_at = Column(DateTime, default=func.now(), onupdate=func.now()) |
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.
А это зачем?
scheduled_requests = Column(Integer, nullable=False, default=0) | ||
success_logins = Column(Integer, nullable=False, default=0) | ||
failed_logins = Column(Integer, nullable=False, default=0) |
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.
Тип лучше установить на BigInteger. Еще лучше сделать проверку на неотрицательность
@@ -0,0 +1 @@ | |||
from .StartMenu import StartMenu |
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.
А где магический __all__
, как и везде в других местах?
|
Начал рефакторить, за пару часов успел сделать следующее:
Оттестированы функции бота, однако уведомления об изменениях не проверены.
Реквест получился большой, лучше спуллить локально и там смотреть изменения.