-
Notifications
You must be signed in to change notification settings - Fork 36
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
Introduce ruff #753
Introduce ruff #753
Conversation
@f213 тегаю, чтобы обратить внимание, т.к. не могу тут назначить ревьюера |
Отдаю @kazqvaizer |
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.
В целом я за, но нужны комменты, возможно даже в ридми, как работать с этим списком исключений.
exclude = ["__pycache__", "migrations"] | ||
line-length = 160 | ||
src = ["src"] | ||
target-version = "py311" |
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.
Из файлов типа .python-versions не умеет цеплять. Но вот с pyproject более менее дружит.
Может работать с явным requires-python из общего конфига проекта. Сделал так.
target-version = "py311" | ||
|
||
[tool.ruff.lint] | ||
ignore = [ |
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.
Хм, а можно TLDR?
Типа эти штуки у нас не использовались? И почему мы не хотим их использовать?
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.
Вот сводка по каждой группе в игноре:
- A... - правила из flake8-builtins, раньше не использовали, в целом, можно и включить
- ANN401 - не использовалось, запрещает typing.Any (ну это перебор на мой взгляд)
- ARG002, ARG005 - не использовалось, и может ломать интрефейсы/полиморфизм
- B019 - запрещает functools.lru_cache/cache в классах, кажется избыточным
- B904 - заставляет кидать исключения строго с from, кажется избыточным
- C408 - не использовалось, заставляет использовать
[](){}
вместо list, tuple, dict . На мой взгляд - это не всегда нужно и текстовые варианты более читаемы бывают. - D... - куча обязаловки для написания docstrings для всех публичных сущностей, может быть нужно для библиотек, но для бизнес кода это оверкил
- DTZ001 - запрещает datetime без таймзоны, кажется избыточным ограничением. Там где нужна таймзона пользуемся джанговским хелпером
- E501 - и так был в игноре, ограничение на макс длину линии
- EM101, EM102 - не нужно, т.к. запрщает писать сообщение ошибки сразу в Excepton, a требует вынести его в отдельную переменную
- FBT - запрещает bool флаги в функциях, в общем то кажется полезным, но не уверен стоит ли включать
- INP - обязаловка иметь init.py , избыточно да и вредно местами
- INT001 - бан f-строк в gettext штуках, кажется не нужным
- N... - куча правил по неймингу аргументов, методов, модулей и прочего, в игноре т.к. конфликтуют с частыми джанговскими патернами типа импорта моделей из apps и подобного
- PERF401 - принуждает к comprehensions вместо for-loop, но иногда это может вредить читаемости, так что игнор
- PGH - обязаловка указывать игноры для линтера и тайпчекера конкретно типа
type: ignore
->type:ignore[override]
, кажется избыточным - PLR0913 - ограничевает количество аргументов в функциях, дефолтно макс 5 аргументов, может быть
- PT001 - раньше тоже игнор был, чтобы использовать pytest.fixture а не pytest.fixture()
- RET501-503 - избыточно, т.к. местами заставляет писать лишние return None, где это и так происходит автоматом.
- RUF015 - избыточно,
next(iter(x))
вместоlist(x)[0]
- S101 - банит использование assert, кажется не нужным ограничением
- S311 - принуждает использовать безопасный random - избыточно и может мешать
- S324 - банит ряд хешфункций, излишне
- SIM102 - ранее тоже было в игноре
- SIM113 - ранее было в игноре
- TC001-003 - принуждает прятать импорты в TYPE_CHECKING всегда, когда возможно - изыбточно, да и ломает lsp в некоторых редакторах
- TRY003 TRY300 - переусложнение работы с исключениями, может еще и навредить
- RUF012 - не очень дружит с django, постоянно требует лишних аннотаций
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 по каждому правилу, как я выше расписал, получится капец как жирно.
В целом, думал, что коммент с основной инфой об исключенном правиле, как это уже сделано, должен уже давать достаточно понимания.
Мб считаешь иначе, помоги тогда придумать формат, как и где лучше дать этот tldr
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.
Спасибо! Когда я говорил про ридми, я имел ввиду указать, что у нас используется ruff и указать, как работать с ним и правильно настраивать самим игнор лист. А то есть ребята, которые этого могут не понять и охуеть от этих ограничений и пошлют нахуй наш темплейт и не будут ставить ему звездочки
@@ -2,6 +2,9 @@ | |||
build-backend = "poetry.core.masonry.api" | |||
requires = ["poetry-core"] | |||
|
|||
[project] | |||
requires-python = "~=3.11" |
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.
Интересно, а у нас нет в шаблоне других хардкодов версии питона? Чтобы их тоже настроить правильно. Может где-то в генерируемом для сервиса CI\CD?
Если нет и это единственное место, то я бы добавил пару уточнений в ридми про то как работать с игнором ruff в нашем проекте и можно мержить
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.
Вообще хардкод версии есть в docker-compose и тут же в pyproject для poetry.
Poetry по-другому не умеет и на requires-python не смотрит :(
А насчет docker compose особо и не знаю, что тут можно сделать, обычно же из env как вариант брать, но тут это не очень в тему.
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.
Походу пора менять poetry на uv https://github.com/astral-sh/uv
@@ -46,7 +46,7 @@ make test # run tests | |||
### Style | |||
|
|||
* Obey [django's style guide](https://docs.djangoproject.com/en/dev/internals/contributing/writing-code/coding-style/#model-style). | |||
* Configure your IDE to use [flake8](https://pypi.python.org/pypi/flake8) for checking your python code. To run our linters manualy, do `make lint`. | |||
* Configure your IDE to use [ruff](https://pypi.org/project/ruff) for checking your Python code. To run our linters manually, do `make lint`. Feel free to [adjust](https://docs.astral.sh/ruff/configuration/) ruff [rules](https://docs.astral.sh/ruff/rules/) in `pyproject.toml` section `tool.ruff.lint` for your needs. |
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.
Добавил немного инфы и ссылок для ruff. Немного отдает rtfm, но для линтера это подходящее настроение 😄
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.
Норм, если тут у тебя все, я смерджу
Ок, тут всё! |
Issue #577
Добавил ruff, во многом вдохновлялся реализацией и конфигом из https://github.com/tough-dev-school/education-backend
Но тут есть несколько отличий:
List, Union, Optional
на актуальные варианты)В остальном всё то же самое.