Refractoring 1 #32
Refractoring 1 #32
Conversation
There was a problem hiding this comment.
Основное, что мне не нравится здесь
- Используется не ORM-way, когда взаимодействуешь с объектами. SQL-way (когда делаешь через select/update) нормальная тема, когда у тебя нет объектов или когда тебе нужны какие-то скаляры. Или когда сериализация в объекты занимает слишком значительное время. В остальных случаях в апишках лучше использовать подход через ORM объекты
- Стиль заметно отличается от других апишек
- Не вижу чтобы изменился воркер. Он не взаимодействовал с объектами из папки Service?
- Тесты не проходят
There was a problem hiding this comment.
Не понял нафига его было переносить, а особенно под routes. Пусть может в корне апишки лежит?
| id_ = await fetcher.create(create_schema.model_dump()) | ||
| return ResponsePostSchema(**create_schema.model_dump(), id=id_) | ||
| q = sa.insert(db_models.Fetcher).values(**create_schema.model_dump()).returning(db_models.Fetcher) | ||
| fetcher = db.session.scalar(q) | ||
| db.session.flush() |
There was a problem hiding this comment.
А почему не как во всех апишках?
from db_models import Fetcher
fetcher = Fetcher(**create_schema.model_dump())
q = db.session.add(fetcher)
db.session.flush()| q = sa.insert(db_models.Fetcher).values(**create_schema.model_dump()).returning(db_models.Fetcher) | ||
| fetcher = db.session.scalar(q) | ||
| db.session.flush() | ||
| return ResponsePostSchema(**create_schema.model_dump(), id=fetcher.id_) |
There was a problem hiding this comment.
А чего не из добавленного в БД, а из аргумента функции?
There was a problem hiding this comment.
Делается через from_attributes в конфиге ResponsePostSchema
| _: dict[str] = Depends(UnionAuth(['pinger.fetcher.read'])), | ||
| ): | ||
| """ | ||
| Возвращает все сборщики метрик. | ||
| """ | ||
| res = await fetcher.get_all() | ||
| return res | ||
| return list(db.session.scalars(sa.select(db_models.Fetcher)).all()) |
There was a problem hiding this comment.
Какая сложная штука
+непонятно что вернет, где схема?
| except exc.ObjectNotFound: | ||
| raise HTTPException(status_code=status.HTTP_404_NOT_FOUND) |
There was a problem hiding this comment.
Давай через Exception handler мб?
| except exc.ObjectNotFound: | ||
| raise HTTPException(status_code=status.HTTP_404_NOT_FOUND) |
| _: dict[str] = Depends(UnionAuth(['pinger.fetcher.read'])), | ||
| ): | ||
| """Получение одного сборщика метрик по id""" | ||
| try: | ||
| res = await fetcher.get_by_id(id) | ||
| q = sa.select(db_models.Fetcher).where(db_models.Fetcher.id_ == id) | ||
| res = db.session.scalar(q) |
There was a problem hiding this comment.
А почему ты забираешь скаляр? Это id возвращает же? Одно значение, не объект. Тут бы хотелось видеть атрибуты фетчера
| if not res: | ||
| raise exc.ObjectNotFound(id) | ||
| q = ( | ||
| sa.update(db_models.Fetcher) |
There was a problem hiding this comment.
Не очень ORM-way, он скорее предполагает что ты получаешь объект Fetcher (db.session.get(Fetcher, id)), редачишь его и после делаешь flush.
| @@ -39,6 +49,7 @@ def test_post_success(crud_client): | |||
|
|
|||
| @pytest.mark.authenticated("pinger.fetcher.read") | |||
| def test_get_by_id_success(crud_client, this_fetcher): | |||
| global fetcher | |||
|
|
||
|
|
||
| metric = {"name": "string", "ok": True, "time_delta": 0} | ||
|
|
||
|
|
||
| @pytest_asyncio.fixture | ||
| async def this_metric(dbsession): | ||
| yield await PgMetricService(dbsession).create(item=metric) | ||
| global metric |
Изменения
Снес все классы сервисов
Убрал ручки alert (и вообще упоминание alert ото всюду)
Удалил асинки
Пофиксил тесты