August 30, 2023

code review // outages-ge-bot

В целом, всё более-менее =) Далее разбил по модулям, чтоб было более конструктивно и понятно. Проект небольшой, но дам советы, которые пригодятся в будущем.

db -> models.py

На каждую модель должен быть отдельный файл. Так и проще для ориентирования и для масштабирования. В файле с моделью у тебя могу лежать и функции и инструменты, которые непосредственно связаны с данной моделью.

В твоём случае должно быть так:

model
chat.py
address.py
sent_outage.py

Заметь, название файла отображает название модели в единственном числе. Название таблицы в множественном - как у тебя и есть.

def default_full_address - у тебя используется в одном месте (дефолт для поля full_address у addresses) и нужна она исключительно для данного поля. Так добавь её в данный класс и объяви приватной (_default_full_address).

Так же начни использовать индексы. В данном случае они вообще не критичны, но для понимания, опыта и ускорения выполнения операций, хоть и на чуть, будет полезно. Подумай сам, по каким уникальным полям будет происходить поиск чаще всего?

Про удаление на примере "Chat"

Вообще, лучше никогда, ничего не удалять с базы. Юзай статусы.
Добавь колону "state" и меняй у неё нужные тебе статусы.

Пример

state = Column(Integer, nullable=False, default=2)

# далее внизу пишешь так:
State = _ChatState()

# и отдельно обьявляешь допустимые статусы:
class _ChatState(Enum):

def __init__(self, **kwargs):
super(_ChatState, self).__init__(**kwargs)
self.active = 1
""" активный """
self.inactive = 2
"""нет активный"""
self.deleted = 3
""" удаленный """
self.banned = 4
"""заблокированный"""

Это к примеру, главное понять суть, что так больше функциональности и гибче код

db -> functions.py

По поводу функций insert_chat, delete_chat, is_address_num_exceeded

А что если тебе нужно добавить или удалить несколько чатов? Допустим в коде тебе надо будет удалить 100 чатов. Ты будешь 100 раз открывать сессию, удалять чат, комитеть там и закрывать сессию?
Лучше добавить функциональность по добавлению и т.д. как одного, так и множества сущностей.

По коду у тебя не возникает такий ситуаций. Но так как ты, допустим, не будешь удалять, а будешь проставлять статус deleted, то можно реализовать следующим образом:

Пример:

def delete_chat(chats: [str]):
    """Delete chats from database"""

    with Session(engine) as session:
        session.begin()
        result_ids = []
        for chat_id in chats: 
            chat = session.query(Chat).filter_by(tg_chat_id=chat_id).one_or_none()
            if chat:
                chat.state = Chat.State.deleted
                result_ids.append(chat.id)
             else:
                 logging.info({chat_id} not found)

        try:
            session.commit()
            logging.info(Total chat was deleted: {len(result_ids)}. Chat: ({result_ids}) was deleted from database.')
        except Exception as err:
            logging.error(f'Chats ({chats}) wasn\'t deleted, {err}')
            return None

scrapers -> gwp.py and telasi.py

На данный момент ни а каком расширении тут и речи идти и не может =)
Заметил, что не используешь классы. Как раз вот тут они нужны.

Давай с очевидного not DRY
collect_outages, request_soup -> полностью дублируются. Даже не применяя классы их можно вынести отделно и импортировать

scrap_notifications, parse_notifications_info -> по большей части копируют друг друга, их вот как раз и нужно юзать в классе.

Тут у нас есть 2 пути:
1) Берём один файл и переписываем там всё под класс меняя вещи, которые будут меняться на атрибуты класса и методы. Далее уже мы наследуемся от этого класса и меняем атрибуты и методы под свой случай. Так же тут добавляем методы, которые именно здесь нужны.

2) Создаём абстрактный класс, там описываем методы, которые в общей случае должны быть, минимальную логику, атрибуты. Далее уже наследуемся от этого класса в конкретном случае. А когда нужно будет добавить новы скраппер, то если сильно прям он отличается, то так же наследуемся от абстрактного, если нет, то от уже существующего класса с логикой и заменяем что нужно.

Конечно, второй вариант предпочтительней и правильней.

То есть выглядеть класс будет примерно так:

Class Gwp:
def __init__(
self,
type: str = 'water'',
root_url: str = "",
urls: list[dict] | None = None):
self.type = 'water'
self.root_url = 'https://www.gwp.ge'
self.urls = [{"url": urljoin(ROOT_URL, '/ka/dagegmili'), "emergency": False},
{"url": urljoin(ROOT_URL, '/ka/gadaudebeli'),"emergency":True}]

И тут уже перечисляешь методы. Например, collect_outages ты укажешь один раз в абстрактном классе и её вообще писать не придётся. Если не хочешь передавать urls как-то в класс, то просто делаешь данный метод @staticmethod

get_info - ты просто создашь новый метод у класса, так как используется только в одном из 2

scrap_notifications - перепишешь в соотв. с нужной логикой.

Вообще, это самый основной момент. Во-первых, самая важная часть. Во-вторых, самая перспективно маштабируемая.

Так же момент по поводу парсинга. Что будет если где-то, что-то на страннице поменяют? Парсер сломается и не всегда ты это сможешь заметить. Поэтому здесь если ты парсишь, что-то связанной со стилями или расположением инфы, то оборачивай или в try/except или обрабатывай результат, который не ожидаешь получить.

То есть если пришло, то что не должно прийти или упала ошибка, то логируй, чтоб ты мог оперативно увидеть это и поправить.

Общее

Ты писал про асинхронность. Я не совсем понял о чём именно ты имел в виду. На созвоне пояснишь тогда, а я расскажу почему так и что можно сделать.
По деплоую так же лучше обсудить. Ну в твоём случае прост через докер запускаешь и всё. Нет необходимости поднимать там сервер или кубы.

В целом с докером, гитигноре и проектом всё норм. Проект небольшой, выполняет свою функцию, написан нормально. Все хорошо =)

Каких-то явных ошибок и косяков нет. Я больше указал по архитектуре, чтоб ты мог масштабировать и сразу понимал в каком направлении думать при создании приложения.
Если используешь аннотацию типов, то и используй при указании возвращаемого значение (def test() -> bool)