Добавлен базовый api для frontend #19

Closed
LazIvanS wants to merge 0 commits from feat8 into develop
LazIvanS commented 2021-04-14 20:34:21 +03:00 (Migrated from github.com)
No description provided.
AlekseyLobanov (Migrated from github.com) requested changes 2021-04-14 20:56:01 +03:00
AlekseyLobanov (Migrated from github.com) left a comment

Довольно много пожеланий по стилю, которые, как мне кажется, стоит поправить.
Также нет поддержки ToDoItem, только списки. Эта поддержка нужна хоть какая-то, даже если методы будут просто заглушками, чтобы GUI можно было писать, опираясь на этот интерфейс

Довольно много пожеланий по стилю, которые, как мне кажется, стоит поправить. Также нет поддержки ToDoItem, только списки. Эта поддержка нужна хоть какая-то, даже если методы будут просто заглушками, чтобы GUI можно было писать, опираясь на этот интерфейс
AlekseyLobanov (Migrated from github.com) commented 2021-04-14 20:37:19 +03:00

Такие импорты, в целом, плохая практика, особенно, на уровне не конечных модулей.
Лучше import json

Такие импорты, в целом, плохая практика, особенно, на уровне не конечных модулей. Лучше `import json`
AlekseyLobanov (Migrated from github.com) commented 2021-04-14 20:38:15 +03:00

Обычно, импорты принято сортировать по порядку: сначала стандартные и системные, потом библиотеки внешние, потом библиотеки внутренние потом какие-то проектные модули.

Обычно, импорты принято сортировать по порядку: сначала стандартные и системные, потом библиотеки внешние, потом библиотеки внутренние потом какие-то проектные модули.
AlekseyLobanov (Migrated from github.com) commented 2021-04-14 20:41:01 +03:00

Лучше в принципе не использовать изменяемые аргументы как аргументы по умолчанию. Но если очень хочется, то можно использовать https://stackoverflow.com/a/52022425/1551213

Лучше в принципе не использовать изменяемые аргументы как аргументы по умолчанию. Но если очень хочется, то можно использовать https://stackoverflow.com/a/52022425/1551213
AlekseyLobanov (Migrated from github.com) commented 2021-04-14 20:42:20 +03:00

Мне кажется, что слишком хитрая штука.
Для работы с url'ами следует использовать urljoin

Мне кажется, что слишком хитрая штука. Для работы с url'ами следует использовать urljoin
AlekseyLobanov (Migrated from github.com) commented 2021-04-14 20:44:05 +03:00

Выбор имён мне кажется странным.
У пользователя есть метод delete, который на самом деле удаляет какой-то список этого пользователя? Это надо как-то улучшить, мне кажется

Выбор имён мне кажется странным. У пользователя есть метод delete, который на самом деле удаляет какой-то список этого пользователя? Это надо как-то улучшить, мне кажется
AlekseyLobanov (Migrated from github.com) commented 2021-04-14 20:46:35 +03:00
  1. Использование кодов возврата в Python мне кажется совсем не-идиоматичным. Функции делают то, что должно им делать. А если не получается, то кидают исключения
  2. Этот код выдаётся наружу, но этот код, 200, не имеет смысла за пределами HTTP, что этот модуль должен, по идее, инкапуслировать
  3. GET в случае успеха возвращает 200, но не другие методы: PUT,POST, DELETE
1. Использование кодов возврата в Python мне кажется совсем не-идиоматичным. Функции делают то, что должно им делать. А если не получается, то кидают исключения 2. Этот код выдаётся наружу, но этот код, 200, не имеет смысла за пределами HTTP, что этот модуль должен, по идее, инкапуслировать 3. GET в случае успеха возвращает 200, но не другие методы: PUT,POST, DELETE
AlekseyLobanov (Migrated from github.com) commented 2021-04-14 20:47:26 +03:00

Вместо такого переопределения, мне кажется, лучше использовать requests.get(json={"x":42}), а он уже сам поставит правильные заголовки

Вместо такого переопределения, мне кажется, лучше использовать `requests.get(json={"x":42})`, а он уже сам поставит правильные заголовки
AlekseyLobanov (Migrated from github.com) commented 2021-04-14 20:48:14 +03:00
        data = dump({"username": user, "password": passwd})
```suggestion data = dump({"username": user, "password": passwd}) ```
AlekseyLobanov (Migrated from github.com) commented 2021-04-14 20:48:44 +03:00
                data = r.json()
```suggestion data = r.json() ```
AlekseyLobanov (Migrated from github.com) commented 2021-04-14 20:49:33 +03:00

Ещё раз напишу, что использование кодов возврата за пределами этого модуля будет плохой идеей, как мне кажется

Ещё раз напишу, что использование кодов возврата за пределами этого модуля будет плохой идеей, как мне кажется
AlekseyLobanov (Migrated from github.com) commented 2021-04-14 20:53:46 +03:00

Использование методов с двойными подчёркиваниями является плохой практикой, т.к. обычно их для себя резервирует Python. Если это закрытый метод, то лучше использовать одинарное нижнее: _do_somthing

Использование методов с двойными подчёркиваниями является плохой практикой, т.к. обычно их для себя резервирует Python. Если это закрытый метод, то лучше использовать одинарное нижнее: `_do_somthing`
AlekseyLobanov (Migrated from github.com) commented 2021-04-14 20:54:25 +03:00

Это точно нельзя хардкодить. Проще всего получать в конструкторе.

Это точно нельзя хардкодить. Проще всего получать в конструкторе.
AlekseyLobanov (Migrated from github.com) commented 2021-04-14 20:52:06 +03:00

В целом, токены JWT можно считать долгоживущими, поэтому их лучше получать только один раз, а потом

  1. Уметь их принимать в конструкторе, чтобы пользователь смог только один раз ввести логин/пароль, а потом использовался бы готовый токен
  2. В случае проблем пробовал бы получать новый токен.
  3. Умел бы целиком работать без логина/пароля [опционально]
  4. Не генерировать токенов на каждый запрос
В целом, токены JWT можно считать долгоживущими, поэтому их лучше получать только один раз, а потом 1. Уметь их принимать в конструкторе, чтобы пользователь смог только один раз ввести логин/пароль, а потом использовался бы готовый токен 2. В случае проблем пробовал бы получать новый токен. 3. Умел бы целиком работать без логина/пароля [опционально] 4. Не генерировать токенов на каждый запрос
AlekseyLobanov commented 2021-04-14 21:06:57 +03:00 (Migrated from github.com)

А pre-commit хук точно отработал?

А pre-commit хук точно отработал?
LazIvanS (Migrated from github.com) reviewed 2021-04-15 12:36:18 +03:00
LazIvanS (Migrated from github.com) commented 2021-04-15 12:36:18 +03:00

Модуль весьма мал, плюс не то чтобы нам требовалось что-то большое от json, однако если поддерживать такой стиль по всему проекту то почему бы и исправить.

Модуль весьма мал, плюс не то чтобы нам требовалось что-то большое от json, однако если поддерживать такой стиль по всему проекту то почему бы и исправить.
LazIvanS (Migrated from github.com) reviewed 2021-04-15 12:36:28 +03:00
LazIvanS (Migrated from github.com) commented 2021-04-15 12:36:28 +03:00

Сохранилось при переходе от urllib к requests

Сохранилось при переходе от urllib к requests
LazIvanS (Migrated from github.com) reviewed 2021-04-15 12:36:34 +03:00
LazIvanS (Migrated from github.com) commented 2021-04-15 12:36:33 +03:00

Ну искать какой-то специальный класс неизменяемого словаря (его именование уже об этом говорит), в принципе не выходящего за пределы своего модуля для одного аргумента не хотелось.

Ну искать какой-то специальный класс неизменяемого словаря (его именование уже об этом говорит), в принципе не выходящего за пределы своего модуля для одного аргумента не хотелось.
LazIvanS (Migrated from github.com) reviewed 2021-04-15 12:36:38 +03:00
LazIvanS (Migrated from github.com) commented 2021-04-15 12:36:38 +03:00

Не попалась по названию (при поиске аналога os.path.join), учтем

Не попалась по названию (при поиске аналога os.path.join), учтем
LazIvanS (Migrated from github.com) reviewed 2021-04-15 12:36:50 +03:00
LazIvanS (Migrated from github.com) commented 2021-04-15 12:36:50 +03:00

Выбор имен на данный момент целиком и полностью соответствует перечню запросов в swagger-е, вместе с развитием понятия что там реализовано будет и развитие документации-названий по тому что реализовано в API.

Выбор имен на данный момент целиком и полностью соответствует перечню запросов в swagger-е, вместе с развитием понятия что там реализовано будет и развитие документации-названий по тому что реализовано в API.
LazIvanS (Migrated from github.com) reviewed 2021-04-15 12:37:11 +03:00
LazIvanS (Migrated from github.com) commented 2021-04-15 12:37:11 +03:00

Про POST и остальные не учел. Добавлять их[коды] тоже не хотелось бы, но показать юзеру почему не проходит его запрос без залезания в код api тоже надо бы. Исключения тут уместными не казались, тк операция может не пройти именно на стороне сервера, но для лаконичности наверно придется общаться через них.

Про POST и остальные не учел. Добавлять их[коды] тоже не хотелось бы, но показать юзеру почему не проходит его запрос без залезания в код api тоже надо бы. Исключения тут уместными не казались, тк операция может не пройти именно на стороне сервера, но для лаконичности наверно придется общаться через них.
LazIvanS (Migrated from github.com) reviewed 2021-04-15 12:37:17 +03:00
LazIvanS (Migrated from github.com) commented 2021-04-15 12:37:17 +03:00

ок

ок
LazIvanS (Migrated from github.com) reviewed 2021-04-15 12:39:49 +03:00
LazIvanS (Migrated from github.com) commented 2021-04-15 12:39:48 +03:00

Принимать в конструкторе токен звучит неплохо. А разве мы генерируем новые на каждый запрос? Проблемы с авторизацией это уже за пределами первой версии api.

Принимать в конструкторе токен звучит неплохо. А разве мы генерируем новые на каждый запрос? Проблемы с авторизацией это уже за пределами первой версии api.
LazIvanS (Migrated from github.com) reviewed 2021-04-15 12:39:54 +03:00
LazIvanS (Migrated from github.com) commented 2021-04-15 12:39:54 +03:00

Да, привычка выделять, стоит использовать одинарные

Да, привычка выделять, стоит использовать одинарные
LazIvanS (Migrated from github.com) reviewed 2021-04-15 12:42:08 +03:00
LazIvanS (Migrated from github.com) commented 2021-04-15 12:42:08 +03:00

Должно было быть в конструкторе

Должно было быть в конструкторе
AlekseyLobanov (Migrated from github.com) reviewed 2021-04-15 14:07:31 +03:00
AlekseyLobanov (Migrated from github.com) commented 2021-04-15 14:07:31 +03:00

Токены нигде не сохраняются, но каждый запрос этот токен требует. Значит генерируем новый токен на каждый запрос

Токены нигде не сохраняются, но каждый запрос этот токен требует. Значит генерируем новый токен на каждый запрос
LazIvanS commented 2021-04-15 20:38:20 +03:00 (Migrated from github.com)

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

Добавил исправления стиля, плюс базовое понимание того что происходит в запросах. Дальше уже пишем "удобные" методы работы со структурами данных, когда будет лучше понятна их структура
AlekseyLobanov (Migrated from github.com) requested changes 2021-04-17 13:42:01 +03:00
AlekseyLobanov (Migrated from github.com) left a comment

Можно уточнить у коллег, но мне кажется, что текущий интерфейс требует изменений.
Если подумать, каким бы был клиент, если бы API уже было готово и было бы большим?
Мне кажется, что это сущности, с которыми мы работаем ApiList, ApiToDoItem (и другие, если бы они были) которые бы могли удаляться/создаваться/обновляться.
При этом они были бы как-то связаны непосредственно с настройками API (модуль-синглтон или отдельный класс), где под настройкой будет подразумеваться работа с транспортом HTTPS, json и JWT токенами.
Тогда эти сущности можно будет без изменений использовать на фронте или с минимальными изменениями.
Также удастся использовать мощную систему типов + возможно, typing

Можно уточнить у коллег, но мне кажется, что текущий интерфейс требует изменений. Если подумать, каким бы был клиент, если бы API уже было готово и было бы большим? Мне кажется, что это сущности, с которыми мы работаем ApiList, ApiToDoItem (и другие, если бы они были) которые бы могли удаляться/создаваться/обновляться. При этом они были бы как-то связаны непосредственно с настройками API (модуль-синглтон или отдельный класс), где под настройкой будет подразумеваться работа с транспортом HTTPS, json и JWT токенами. Тогда эти сущности можно будет без изменений использовать на фронте или с минимальными изменениями. Также удастся использовать мощную систему типов + возможно, typing
AlekseyLobanov (Migrated from github.com) commented 2021-04-17 13:35:27 +03:00

Я придерживаюсь позиции, что лучше нормальные имена всегда, когда возможно, чем документация.
Также принято, что комментарии лучше использовать для ответа на вопрос "Зачем?", а не на вопрос "Что происходит?".
Например, переименовать
def delete(self, id):
в
def delete_list(self, list_id)
Заодно удастся избавиться от перекрытия имени id, что тоже не очень хорошо

Я придерживаюсь позиции, что лучше нормальные имена всегда, когда возможно, чем документация. Также принято, что комментарии лучше использовать для ответа на вопрос "Зачем?", а не на вопрос "Что происходит?". Например, переименовать ` def delete(self, id):` в `def delete_list(self, list_id)` Заодно удастся избавиться от перекрытия имени `id`, что тоже не очень хорошо
AlekseyLobanov (Migrated from github.com) commented 2021-04-17 13:33:01 +03:00

Тут мы говорим, что токен это словарь, т.е. как бы отказываемся от проверки типов. Типы нужны, чтобы их проверять. Я бы предложил принимать два токена: на доступ и на обновление (опционален). Тогда удастся использовать более жёсткие контракты без потери какой-либо гибкости

Тут мы говорим, что токен это словарь, т.е. как бы отказываемся от проверки типов. Типы нужны, чтобы их проверять. Я бы предложил принимать два токена: на доступ и на обновление (опционален). Тогда удастся использовать более жёсткие контракты без потери какой-либо гибкости
LazIvanS (Migrated from github.com) reviewed 2021-04-17 14:36:47 +03:00
LazIvanS (Migrated from github.com) commented 2021-04-17 14:36:47 +03:00

Это имя соответствует запросу из сваггера, более того то что делает этот запрос это все еще догадка

Это имя соответствует запросу из сваггера, более того то что делает этот запрос это все еще догадка
LazIvanS (Migrated from github.com) reviewed 2021-04-17 14:37:37 +03:00
LazIvanS (Migrated from github.com) commented 2021-04-17 14:37:37 +03:00

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

Предполагается что юзер не знает об апи и формате токенов и пользуется только теми токенами что оно ему предоставляет. Но немного больше логики - да, можно
LazIvanS commented 2021-04-17 14:39:31 +03:00 (Migrated from github.com)

Текущие запросы в апи - ниузкоуровневые, 1-1 запросы. Тогда добавлю систему классов для работы с более высокоуровневыми командами.

Текущие запросы в апи - ниузкоуровневые, 1-1 запросы. Тогда добавлю систему классов для работы с более высокоуровневыми командами.

Pull request closed

Sign in to join this conversation.