Pylint: детальная проверка работы анализатора кода

Когда Люк работал с Flake8 и одновременно присматривался к Pylint, у него сложилось впечатление, что 95% ошибок, выдаваемых Pylint, были ложными. У других разработчиков был иной опыт взаимодействия с этими анализаторами, поэтому Люк решил детально разобраться в ситуации и изучить его работу на 11 тыс. строк своего кода. Кроме того, он оценил пользу от Pylint, рассматривая его как дополнение к Flake8.



Люк (Luke Plant) — один из британских разработчиков, на чью статью с разбором популярных анализаторов кода мы недавно наткнулись. Линтеры изучают код, помогают найти ошибки и сделать его стилистически согласованным со стандартами и кодом, который пишут разработчики в вашей команде. Самые распространенные из них — Pylint и Flake8. Мы в Leader-ID их тоже используем, потому с радостью сделали перевод его статьи. Надеемся, что она обогатит и ваш опыт работы с этими инструментами.

Начальные установки и тестовая база

Для данного эксперимента я взял часть кода из одного своего проекта и запустил Pylint с базовыми настройками. Затем попытался проанализировать результат: какие предупреждения оказались полезными, а какие ложными.

Небольшая справка о проекте, из которого был взят код:

• Обычное приложение, написанное на Django (т.е. внутри все тот же Python). У Django есть свои особенности, и, как фреймворк, он имеет свои ограничения, но позволяет писать нормальный код на Python. Некоторые его недостатки как фреймворка также есть и у других библиотек, использующих шаблоны (callback-функций или шаблоны проектирования Template Method).
• Состоит из 22 000 строк кода. Через Pylint прошло примерно 11 000 строк (9 000, если отбросить пропуски). Эта часть проекта состояла преимущественно из кода views и тестового кода.
• Для анализа кода этого проекта я уже использовал Flake8, обработав все полученные ошибки. Смысл этого эксперимента состоял в том, чтобы оценить пользу от Pylint, как прибавку к Flake8.
• У проекта хорошее тестовое покрытие кода, но так как я его единственный автор, у меня не было возможности воспользоваться коллегиальным рецензированием.

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

Итак, Pylint выдал 1650 претензий к моему коду.

Ниже я сгруппировал все замечания анализатора по типам и дал к ним свои комментарии. Если вам нужно подробное описание этих сообщений, загляните в соответствующий раздел списка функций Pylint.

Баги

Pylint нашел ровно один баг в моем коде. Багом я считаю ошибку, которая возникает или может потенциально возникнуть во время работы программы. В этом кейсе я использовал исключения — broad-except.То есть except Exception, а не просто except, который Flake8 отлавливает. Это повлекло бы за собой неправильное поведение во время выполнения при наличии некоторых исключений. Если бы эта ошибка когда-либо выскочила во время выполнения (не факт, что выскочит), то неверное поведение кода не вызвало бы серьезных последствий, хотя…

Итого: 1

Полезное

В придачу к той ошибке Pylint нашел еще несколько, которые я отнес к категории «полезное». Код от них не падал, но могли возникнуть проблемы при рефакторинге, да и в принципе к ошибкам в будущем при расширении списка фичей и поддержке кода.

Семь из них были too-many-locals / too-many-branches / too-many-local-variables. Они относились к трем частям моего кода, которые были плохо структурированы. Над структурой хорошо бы было еще подумать, и я уверен, что мог бы сделать лучше.

Остальные ошибки:

• unused-argument × 3 — один из них действительно был косяком, и код выполнялся правильно случайно. Другие два ненужных и неиспользуемых аргумента привели бы к проблемам в будущем, если бы я их использовал.
• redefined-builtin × 2
• dangerous-default-value × 2 — не баги, ибо я никогда не использовал дефолтные значения, но хорошо бы это исправить на всякий случай.
• stop-iteration-return × 1 — вот тут я узнал для себя что-то новое, никогда бы сам не нашел.
• no-self-argument × 1

Итого: 16

Косметические правки

На эти вещи я бы обращал меньше внимания. Они либо незначительные, либо маловероятные. С другой стороны, их исправление лишним не будет. Часть из них — спорные стилистические. О некоторых похожих косяках я рассказывал в других разделах, но те, что будут тут перечислены, подходят и под этот контекст. Используя регулярно Pylint, я бы поправил эти «недочеты», но в большинстве случаев не стал бы беспокоиться о них.

invalid-name × 192

Это были в основном имена переменных из одной буквы. Использовал в тех контекстах, где это было не страшно, например:

или

Многие были в коде тестов:

• len-as-condition × 20
• useless-object-inheritance × 16 (наследие Python 2)
• no-else-return × 11
• no-else-raise × 1
• bad-continuation × 6
• redefined-builtin × 4
• inconsistent-return-statements × 1
• consider-using-set-comprehension × 1
• chained-comparison × 1

ИТОГО: 252

Бесполезное

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

• too-many-ancestors × 76

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

• unused-variable × 43

Встречалось почти все время в тестовом коде, где я разбивал запись:

… и не использовал ни один из элементов. Есть несколько способов сделать так, чтобы Pylint тут не сообщал об ошибках (например, дать названия unused). Но если оставить в том виде, в котором я написал, он будет читабельным, и люди (в том числе я) смогут его понимать и поддерживать.

• invalid-name × 26

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

• redefined-outer-name × 16

Иногда имя переменной указано правильно как для внутреннего, так и для внешнего контекста. И вам никогда не придется использовать внешнее имя из внутреннего контекста.

• too-few-public-methods × 14

Примеры включают классы с данными, созданными при помощи attrs, где нет публичных методов, и класс, который реализует интерфейс словаря, но который нужен, чтобы обеспечить корректную работу метода __getitem__

• no-self-use × 12

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

• attribute-defined-outside-init × 10

В этих случаях были веские причины написать код как он есть. В основном эти ошибки возникали в коде тестов.

• too-many-locals × 6, too-many-return-statements × 6, too-many-branches × 2, too-many-statements × 2

Да, эти функции были слишком длинными. Но посмотрев на них, я не увидел хороших способов их подчистить и улучшить. Одна из функций была простой, хотя и длинной. Она имела очень четкую структуру и не была криво написанной, а любые способы ее сокращения, которые я мог бы придумать, включали бы бесполезные слои лишних или неудобных функций.

• arguments-differ × 6

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

• ungrouped-imports × 4

Я уже использую isort для импорта

• fixme × 4

Да, есть несколько вещей, которые надо исправить, но прямо сейчас исправлять их не хочу.

• duplicate-code × 3

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

• broad-except × 2
• abstract-method × 2
• redefined-builtin × 2
• too-many-lines × 1

Я пытался придумать каким естественным способом разбить этот модуль, но не смог. Это один из примеров, где видно, что линтер — неправильный инструмент. Если у меня есть модуль с 980 строками кода, и я добавлю еще 30, я пересекаю лимит в 1000 строк, и уведомления от линтера мне тут ничем не помогут. Если 980 строк — это нормально, то почему 1010 плохо? Я не хочу рефакторить этот модуль, но хочу, чтобы линтер не выдавал ошибки. Единственным решением в этот момент я вижу сделать как-то так, чтобы линтер замолчал, а это противоречит конечной цели.

• pointless-statement × 1
• expression-not-assigned × 1
• cyclic-import × 1

С циклом разобрались перемещением его части в одну из функций. Я не мог найти лучшего способа структурировать код с учетом ограничений.

• unused-import × 1

Я уже добавлял # NOQA при использовании Flake8, чтобы эта ошибка не выскакивала.

• too-many-public-methods × 1

Если в моем тестовом классе 35 тестов вместо 20 регламентируемых, неужели это действительно проблема?

• too-many-arguments × 1

Итого: 243

Невозможно поправить

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

• unused-argument × 21
• invalid-name × 13
• protected-access × 3

Включало доступ к «документированным внутренним объектам», таким как sys._getframe в stdlib и Django Model._meta.

• too-few-public-methods × 3
• too-many-arguments × 2
• wrong-import-position × 2
• attribute-defined-outside-init × 1
• too-many-ancestors × 1

Итого: 46

Ложные сообщения

Вещи, в которых Pylint явно неправ. В данном случае это не ошибки Pylint: дело в том, что Python динамичен, а Pylint пытается обнаружить вещи, которые невозможно сделать идеально или надежно.

• no-member × 395

Связано с несколькими базовыми классами: из Django и теми, которые я создал сам. Pylint не смог обнаружить переменную из-за динамизма / метапрограммирования.

Многие ошибки возникли из-за структуры кода тестов (я использовал шаблон от django-functest, который в некоторых случаях можно было поправить, добавив дополнительные базовые классы с помощью «абстрактных» методов, которые вызывают NotImplementedError) или, возможно, переименовав многие тестовые классы (я не стал этого делать, потому что в некоторых случаях это бы запутывало).

• invalid-name × 52

Проблема возникала в основном потому что Pylint применил правило PEP8 о константах, считая, что каждое имя верхнего уровня, определенное с помощью =, является «константой». Определить точно, что мы подразумеваем под константой, сложнее, чем кажется, но это не относится к некоторым вещам, которые по своей природе являются константами, например, к функциям. Также правило не должно применяться к менее привычным способам создания функций, например:

Некоторые примеры — дискуссионные из-за отсутствия определения того, что такое константа. Например, следует ли считать константой экземпляр класса, определенный на уровне модуля, который может иметь или не иметь изменяемое состояние? Например, в этом выражении:

• no-self-use × 23

Pylint неправильно заявил Method could be a function для множества случаев, где я использую наследование для выполнения различных реализаций, соответственно, я не могу преобразовать их в функции.

• protected-access × 5

Pylint неверно оценил, кто был «владельцем» (текущий фрагмент кода создает protected атрибут объекта и использует его локально, но Pylint этого не видит).

• no-name-in-module × 1
• import-error × 1
• pointless-statement × 1

Это утверждение на самом деле давало результат:

Я использовал это, чтобы намеренно вызвать необычную ошибку, которая вряд ли будет найдена тестами. Я не виню Pylint в том, что он это не распознал…

Итого: 477

Промежуточный итог

Мы еще не на финише, но самое время сгруппировать наши результаты:

1. «Хорошо» — блоки «Баги» и «Полезности» — тут Pylint определенно помог: 17.
2. «Нейтрально» — «Косметические правки» — незначительная польза от Pylint, ошибки не причинят урон: 252.
3. «Плохо» — «Бесполезное», «Невозможно поправить», «Неточности» — там, где Pylint хочет изменений в коде, где этого не требуется. В том числе там, где правки нельзя внести из-за внешних зависимостей или где Pylint просто неверно проанализировал код: 766.

Соотношение хорошего к плохому очень мало. Если бы Pylint был моим коллегой, помогающим делать ревью кода, я бы умолял его уйти.

Чтобы убрать ложные уведомления, можно заглушить вылетающий класс ошибок (что тогда повысит бесполезность использования Pylint) или добавить специальные комментарии в код. Последнее мне не хочется делать, потому что:

1. Это занимает время!
2. Мне не нравятся нагромождения из комментариев, которые существуют для того, чтобы заставить замолчать линтер.

Я с радостью добавлю эти псевдокомментарии, когда от линтера будут несомненный плюс. Кроме того, я трепетно отношусь к комментариям, поэтому моя подсветка синтаксиса отображает их ярко: так, как рекомендовано в этой статье. Тем не менее, в некоторых местах я уже добавил комментарии #NOQA для заглушения Flake8, но с ними для одной секции можно добавить лишь пять кодов ошибок.

Docstrings

Остальные проблемы, которые обнаружил Pylint — пропущенные строки документации. Я вынес их в отдельную категорию, потому что:

1. Они очень спорны, и у вас может быть совсем другая политика в отношении таких вещей.
2. У меня не было времени провести анализ всех их.

Всего Pylint обнаружил 620 недостающих докстрингов (в модулях, функциях, методах классов). Но во многих случаях я оказался прав. Например:

• Когда из названия уже все понятно. Например:
• Когда docstring уже определена — например, если я реализую интерфейс Django’s database router. Добавление своей строки в данном случае может быть опасным.

В остальных случаях моему коду не помешали бы эти строки описаний. Примерно в 15–30% случаях, найденных Pylint, я бы подумал «Да, надо добавить здесь docstring, спасибо Pylint за напоминание».

В целом мне не нравятся инструменты, которые заставляют добавлять докстринги везде и всюду, так как считаю, что в этом случае они будут получаться плохими. Лучше уж не писать их вовсе. Аналогичная ситуация с плохими комментариями:

• их читать — трата времени: в них нет дополнительной информации или они содержат некорректную информацию,
• они помогают на подсознательном уровне выделять докстринги в тексте, поскольку в них содержится полезная информация (если вы пишите их по делу).

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

Заключение

Я считаю, что мои изначальные посылы о бесполезности Pylint оказались верны (в контексте той кодовой базы, для которой я использую Flake8). Чтобы я стал использовать его, показатель, отражающий количество ложные срабатываний, должен быть ниже.

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

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

Другой подход заключается в использовании Pylint для ограниченного числа видов ошибок. Однако таких было лишь несколько, срабатывания на которых оказывались правильными или крайне редко ложными (в относительном и абсолютном выражении). Среди них: dangerous-default-value, stop-iteration-return, broad-exception, useless-object-inheritance.

В любом случае я надеюсь, что эта статья помогла вам при рассмотрении вопроса об использовании Pylint или в споре с коллегами.