HH-168926 add some linters #651
Conversation
| [tool.ruff] | ||
| line-length = 120 | ||
| target-version = 'py311' | ||
| select = [ |
There was a problem hiding this comment.
есть еще интересные правила, которые я бы добавил, но видимо отдельно, т.к. они требуют рефакторинга, а пр и без этого уже очень жирный
правила C90, S, ANN, BLE, FBT, A, SLF, ARG, PGH, PL, TRY
https://docs.astral.sh/ruff/rules/
|
большинство правок сделал автоматом |
frontik/app.py
Outdated
| self.upstream_update_listener = None | ||
| self.available_integrations: list[integrations.Integration] = [] | ||
| self.tornado_http_client: AIOHttpClientWrapper | None = None | ||
| self.http_client_factory: HttpClientFactory = None # type: ignore |
There was a problem hiding this comment.
тут где-то 30-50 таких ситуаций, что в ините переменная None, а дальше в initialize в нее что-то присваивается и дальше у нас либо там не None, либо приложение не запустилось, но mypy об этом не знает
не придумал как красиво обойти, поставил игнор коммент
еще варианты
все порефакторить и вынести инициализации в иниты
во всех юсаджах по коду добавить много if var is not None:
There was a problem hiding this comment.
А нельзя тут просто без инициализации задекларировать? Или None где-то все-таки используется?
There was a problem hiding this comment.
Я бы вместо игнора — все-таки использовал бы Any, чтобы в местах использования было понятно, что хз какой тут тип, потому что сейчас, если даже при изменениях что-то сломается или пойдет не так, то код снаружи будет считать, что с типами все ОК
There was a problem hiding this comment.
в некоторых местах получилось
еще много других тайпигноров осталось
- os.pipe2 на маках нету pipe2
- заоверрайдили торнадо метод с другим порядком аргументов и у нас уже много кода на наш порядок завязано
- неправильные тайпхинты в торнадо. (например, в реквесте хост не может быть None, а по хинту может, или что нельзя корутины передавать в add_future)
- когда манкипатчим методы или аттрибуты перезаписываем
- в конфиге, который мы получаем через exec/eval у нас могут быть какие угодно поля (например config.serviceHost)
- с декларациями не везде решилось, options.app у нас либо прочитается из конфиг файла, либо sys.exit(1), а mypy думает что оно везде str|None
frontik/auth.py
Outdated
| if not debug_access: | ||
| handler.set_header('WWW-Authenticate', f'{header_name}-Header realm="Secure Area"') | ||
| handler.set_status(http.client.UNAUTHORIZED) | ||
| raise DebugUnauthorizedError() | ||
| raise DebugUnauthorizedError |
There was a problem hiding this comment.
разве не нужно объект создавать?
There was a problem hiding this comment.
необязательно, я тоже привык когда с ()
можно вырубить правило
'why its bad' слабенький
https://docs.astral.sh/ruff/rules/unnecessary-paren-on-raise-exception/
There was a problem hiding this comment.
давай вырубим, говеное какое-то правило
| from frontik.util import generate_uniq_timestamp_request_id, check_request_id | ||
| from frontik.version import version as frontik_version | ||
| from frontik.service_discovery import UpstreamCaches, get_async_service_discovery, get_sync_service_discovery | ||
| from frontik.util import check_request_id, generate_uniq_timestamp_request_id | ||
|
|
||
| app_logger = logging.getLogger('http_client') | ||
|
|
||
| if TYPE_CHECKING: |
There was a problem hiding this comment.
А если у нас уже питон 3.11 то этот гард все еще актуален?
There was a problem hiding this comment.
где только из typing импортим можно и снять
но в некоторых местах без этого циклические импорты получаются
frontik/app.py
Outdated
|
|
||
| def application_404_handler(self, request): | ||
| def application_404_handler(self, request: HTTPServerRequest) -> tuple: |
There was a problem hiding this comment.
Вот в таких местах бы как-то сделать так, чтобы написанный возвращаемый тип был не шире, чем тот, который вычисляется автоматом
А то раньше редактор для этого метода подсказывал tuple[ErrorHandler, dict], а теперь tuple[Any, Any]
There was a problem hiding this comment.
настоящий тип шире
типа как минимум tuple[type[PageHandler], dict]
здесь получилось указать, в некоторых местах если явно прописать то начинаются теже проблемы как выше говорил
| setattr(options, name, value.lower() not in ("false", "0", "f")) | ||
| elif option.type == int: | ||
| elif option.type == int or get_args(option.type) == (int, type(None)): | ||
| setattr(options, name, int(value)) |
There was a problem hiding this comment.
что этот код вообще делает?
а вот если тут выпонлится get_args(option.type) == type(None)
разве NPE не стрельнет внутри на int(None)?
There was a problem hiding this comment.
этот код парсит конфиги и перекладывает их в объект options https://github.com/hhru/frontik/blob/master/frontik/options.py#L9
тут конечно напрашивается рефакторинг, но пр итак уже очень большой
по некоторым причинам любые значения считываются как строки, хотя мы в датаклассе хотели бы другой тип, ну и в случае с опциями some_port, там потом еще и код упадет если это строкой останется
так что в этом участке смотрим, что ждали в датаклассе и пытаемся скастить в нужный тип
раньше было ок иметь в дата классе int и кастить в int
а теперь в датаклассе бывают some_port: int | None и их тоже нужно кастить в int,
как понять что опция именно этого типа - проверить что тип вот такой тупл из (int, none)
712u3
force-pushed
the
HH-168926
branch
2 times, most recently
from
de8974f
to
28a8c05
Compare
|
SonarQube Quality Gate
|
https://jira.hh.ru/browse/HH-168926