Хочу поделиться своим успешным опытом внедрения автоматической проверки стилей кода в проект и рассказать на какие грабли наступали. Опубликовать opensource сообществу инструменты, которые были созданы для решения задачи
Немного о нашем проекте: PHP сайт, лежит в git репозитории объемом 2Gb, состоит из 20k php файлов, возраст проекта- 10 лет, в данный момент у нас 15 разработчиков. Для code review используем atlassian stash. Всю разработку ведем в рамках отдельных веток, которые после прохождения code review вливаем в master и деплоим на прод
Для начала вопрос: а зачем? Зачем вообще следить за стилями кода?
Ответ для меня простой: код пишется один раз, а читается потом десятками людей на протяжении многих лет. Следовательно стоимость поддержки кода многократно превышает стоимость написания. А раз так — то стоит потратить немного времени и отформатировать код согласно некому стандарту
Первое на что наткнулись: а какой стандарт выбрать? Поступили так — создали свой стандарт, унаследованный от symfony, и вносим туда изменения согласно нашему представлению о прекрасном. Некоторые правила мы просто выбрасывали, как невозможные в нашем проекте для старых классов (например, то что любой класс должен быть внутри namespace). Благо phpcs представляет хорошие возможности по настройке стилей кода
Второе: а чем проверять код? Какие есть утилиты для этого? После недолгих поисков остановились на phpcs, так как есть нативная поддержка в phpstorm
Третье: а что проверять? Весь проект у нас огромный, писался десятилетие многими поколениями разработчиков. Ошибок в стилях там просто немерено. Потому решили, что проверять нужно только те строки, которые изменил разработчик в рамках данной задачи
Третье: а как собственно проверять код?
Тут были разные варианты
- Как известно, git поддерживает серверные хуки. Можно проверять стили на стороне сервера и не давать запушить, пока ошибки не будут исправлены. Но такой подход мне не понравился, так как бодаться пушами с гитом в консоли не очень-то удобно :) Плюс тут невозможно сделать обходной путь, когда исправить все ошибки в стилях по каким-то причинам невозможно (например, нужно переименовать имя метода, который используется в 100500 файлах)
- После некоторых размышлений решили, что идеальный вариант, когда робот будет сам отписываться в pull request про найденные ошибки в стилях, которые уже разработчик должен будет исправлять. При этом на первом этапе мы разрешали какие-то ошибки не исправлять, если это требовало существенно рефакторинга, дорабатывали удобные для нас правила проверки. Так родился инструмент — phpcs-stash
Немного об архитектуре инструмента:
1) При пуше в некоторую ветку webhook дергает phpcs-stash и передает название ветки, куда был сделан push
2) phpcs-stash запрашивает по API у stash все рецензии (pull requests) с этой веткой, проходится по всем файлам (дергает их так же из stash по API), проверяет стили, отписывается где нашел ошибку. В случае если ошибок нет — аппрувит рецензию. Отписывается робот только в измененных строках
Результат выглядит вот так:
3) При повторном пуше робот заново проверяет все рецензии, удаляет комментарии об исправленных ошибках
Для любителей картинок:
При таком подходе у нас остается возможность настройки родной интеграции между stash и jira (например: запретить закрывать задачу если рецензия кода не одобрена роботом). И есть отличная возможность анализировать какие ошибки случаются чаще всего, что бы дорабатывать правила проверки
Так как система проверки не знает ни о чем кроме atlassian stash, было решено сделать её ориентированной в open source. Изначально делалось только для PHP, но парням из соседнего отдела понравилось и они сделали поддержку проверки C++ кода
Для быстрой обратной связи используется phpstorm, который умеет интегрироваться с phpcs. Выглядит вот так:
Что дальше?
Есть идея сделать проверяльщик интерактивным: чтобы можно было ответить на комментарий робота «исправь», и робот бы сам исправил ошибку, если её можно исправить (например, отсутствующий пробел). Но, к сожалению, я не нашел инструментов, которые могут исправлять стили в конкретной строке. Если сообщество подскажет — буду рад
Ссылки:
- phpcs-stash — https://github.com/WhoTrades/phpcs-stash
- REST API Atlasisan Stash — https://developer.atlassian.com/stash/docs/latest/reference/rest-api.html
Комментарии (31)
Borro
28.06.2016 08:00Еще можно добавить к валидации https://phpmd.org/ — помогает писать более чистый код. Правда я бы подкрутил его настройки, например CyclomaticComplexity, NPathComplexity.
Ents
28.06.2016 08:10Да, это есть в планах. Когда буду делать — оформлю в виде отдельной утилиты, по аналогии с phpcs-stash
maximkou
28.06.2016 08:04В readme.MD у вас опечатка — brach вместо branch.
И еще немного провоцирующий вопрос: а какой стандарт стиля установлен в самом инструменте? (просто по коду местами используются разные стили написания). Чтобы не быть голословным: везде отступы пробелами, а например здесь — табами.Ents
28.06.2016 08:14спасибо, опечатку и ошибку в стиле исправил
Как такой стиль кода не задан, но я по привычке пишу в symfony code style http://symfony.com/doc/current/contributing/code/standards.htmlmaximkou
28.06.2016 08:20+4Исправьте уж везде тогда) А не только там где я указал)
Как такой стиль кода не задан
Это конечно сугубо мое личное мнение, но раз уж инструмент следит за код-стайлом, он сам должен не иметь вопросов по тому самому код-стайлу.
Вы же не будете пользоваться, например инструментом тестирования, если он сам не оттестирован?
andrewnester
28.06.2016 08:46+1У нас похожая ситуация с проверкой кода. На каждый пулл риквест запускается задача на CI сервере на проверку стиля. Если есть нарушения, пулл риквест будет иметь статус failed и бот отпишет где и в каких местах проблемы.
Плюс написан свой линтер, который у разработчиков вызывается локально на pre-commit hook
Всё круто работает, иногда правда с легаси кодом проблемы бываютEnts
28.06.2016 09:11А проверяется только измененный код в рамках данной задачи, или весь проект целиком?
LionAlex
28.06.2016 09:22Есть идея сделать проверяльщик интерактивным: чтобы можно было ответить на комментарий робота «исправь», и робот бы сам исправил ошибку
А не боитесь, что он там вам накоммитит не того?
Какой-то странный у вас кодстайл :)
https://github.com/WhoTrades/phpcs-stash/blob/master/lib/Checker/PhpCs.php#L11
class PhpCs implements CheckerInterface { /** * @var \PHP_CodeSniffer */ private $phpcs; /** * @var Logger */ private $log; /** * @param Logger $log * @param array $phpcs - ['encoding' => '....', 'standard' => '...'] */ public function __construct(Logger $log, array $config) {
Ой
var_export($core->runSync('refs/heads/feature/WTA-623', 'WT', 'sparta'));Ents
28.06.2016 11:28Да, часть кода писалась в nodepad++, а там ставится таб, а не пробел. Потому вперемешку. Исправил
По поводу var_export() — app.php это пример использования библиотеке, для самой простой интеграции — webhook. У нас в живом проекте phpcs-stash подключается с помощью composer и работает внутри очереди
minamoto
04.07.2016 14:07Там есть опция специальная, позволяющая сразу при написании заменить таб необходимым количеством пробелов. Очень удобно, рекомендую.
LionAlex
28.06.2016 09:38Зачем дергать напрямую
$GLOBALS['PHP_CODESNIFFER_CONFIG_DATA'], если естьPHP_CodeSniffer::setConfigData( string $key, string|null $value, [boolean $temp = false])?
У вас в код-стайле нет ограничения длинны метода? Кажется,
RequestProcessor::processRequestизлишне жирный и слишком много на себя берет.
А как-же юнит-тесты? :)
А вообще, спасибо, инструмент интересный и полезный в вашем случае. К счастью, у нас можно пофашиствовать и использовать хуки.
heilage
1) А чем пре-коммит хуки не понравились? Или у вас в проекте настолько легаси, что файлы по 4000+ строк?
2) Не задумывались над тем, чтобы прогнать автоматическое переформатирование кода на всем проекте? Один раз и навсегда.
Ents
1) Пользоваться не так удобно как с web интерфейсе. Хук можно отключить и забыть об этом. В качестве быстрой проверки используются стандартный проверщик phpstorm (там тоже phpcs под капотом)
2) Задумывались, но отказались, так как ка хотели терять авторов последних строк
LionAlex
1) Проблемы теже, что и с pre-recieve, да еще и у всех настраивать надо
2) А кто потом это вычитывать будет и смотреть, что ничего не сломалось?
flr
А какие проблемы есть с pre-receive?
Ents
Во-первых — удобство (читать diff с подсветкой удобнее, чем просто список строк с ошибками в консоли)
Во-вторых — рубится на корню всякая возможность проигнорировать предупреждения проверщика стилей (а это бывает необходимо, когда исправление стилей требует исправления сотен классов, скажем, для переименовывания метода, который много где переопределяется)
А в данном случае все ошибки на этапе code review видны всем разработчикам, и они прям в комментариях могут договориться что какая-то ошибка не может быть исправлена и проигнорировать её
heilage
1) Кажется, что в среднем пре-пуш (или интеграция проверок на ide) дает более быструю обратную связь разработчику. Можно использовать в совокупности с пре-ресив хуками, конечно.
2) Автоматическое переформатирование должно гарантировать корректность кода. Если оно не может гарантировать корректность в каких-то специфических случаях — оно не должно трогать код и должно требовать от разработчика правильным образом его исправить.
Вот с частичной потерей истории в git — это да, существенный момент. Однако по себе могу сказать, не так уж оно и нужно — в один прекрасный момент поймал себя на мысли, что лезу в blame чисто посмотреть, кто такую херню написал :) Зачем, почему, как исправить — как правило понятно из кода (а если непонятно, то есть комментарии). Так или иначе, в пушистом легаси коде отсутствие комментариев и неявность намерений исходного автора может действительно быть существенной причиной не делать авто-преобразований.
Ents
Для быстрой обратной связи существует phpstorm, где прямо в момент написания кода подсвечиваются ошибки
OnYourLips
> А чем пре-коммит хуки не понравились?
Потому что они работают на машине разработчика, где ОС и окружение рандомные, а PHP и вовсе может не стоять (у меня нет PHP на рабочей машине, только в виртуалках для проектов, причем для разных проектов — разные версии, разные настройки и т.д.).
А код выполняется на личной виртуалке разработчика с приближенным к продакшену окружением.
Поэтому хук должен стоять на стороне сервера, а не клиента.
heilage
Они работают там, откуда вы коммитите код. Ничто не мешает коммитить с личной разработческой виртуалки, где есть всё, что нужно.
OnYourLips
Виртуалка — это часть проекта. Репозиторий же находится на рабочей машине, где находятся все рабочие инструменты, включая IDE и git.
И запускать рабочие инструменты внутри виртуалки — это какое-то извращение.
Ents
главная ценность — простота и удобство пользования
Поддержка 15 виртуалок разработчиками (по одной виртуалке на каждого разработчика) — это существенная дополнительная работа, которая исключается в случае наличия централизованного инструмента
xotey83
Если у вас есть инструмент поддержики конфигурации а-ля puppet, то проблемы нет.
Sannis
Вы разрешите доступ к своему ноутбуку и виртуалке на нём админскому паппету? Думаю нет :-)
xotey83
Не к своему ноутбуку, а к виртуалке, на которой подняты все сервисы для разработки: php-fpm, nginx, mysql (для экспериментов), xdebug, туда же приностися конфигурация приложения (мы используем подобие etcd), и прочее. Вот эта виртуалка и управляется папетом.
А ноутбук используется в качестве тонкого клиента: phpstorm, браузер и ssh-клиент.
UPD. Уточню: виртуалка находится не на ноутбуке, а на девелоперском сервере.