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

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

597aaa85ab274874e38ce3b6146666d6.png


Люк (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

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

94f833bf1008d8403a0e44716852c587.png


или

e439a275ceeb9508d0566015a7875e3b.png


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

  • 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


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

2c72061bd4270ea1128d3ea63a26e18a.png

… и не использовал ни один из элементов. Есть несколько способов сделать так, чтобы 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 о константах, считая, что каждое имя верхнего уровня, определенное с помощью =, является «константой». Определить точно, что мы подразумеваем под константой, сложнее, чем кажется, но это не относится к некоторым вещам, которые по своей природе являются константами, например, к функциям. Также правило не должно применяться к менее привычным способам создания функций, например:

4d39450fdda92751a828ba4344e309b5.png

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

c56a2652fd28c08feee8306c6c96a048.png
  • 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


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

4549c43ea3cdb8f7910eaa2edce758cd.png


Я использовал это, чтобы намеренно вызвать необычную ошибку, которая вряд ли будет найдена тестами. Я не виню 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 или в споре с коллегами.

© Habrahabr.ru