Headhunter — продуктовая компания, нам очень важно качество кода. Чем он лучше, тем быстрее мы можем выпускать новые бизнес фичи и чаще радовать пользователей.
Для каждого пулл реквеста нужно обязательно пройти ревью, даже если изменена всего одна строчка. Необходим аппрув как минимум одного человека, ревью при этом открытое, кто угодно может участвовать, и это приветствуется. Ревью необходимо для повышения качества кода и распространения знаний между различными командами.
Раньше на ревью были постоянные споры о том, как и что правильно писать. На это уходило много времени и сил. Для решения этих проблем был написан Style Guide. Он сильно помог, так как появился источник правды, на который всегда можно сослаться. Также Style Guide помогает новичкам входить в проект, сразу объясняя, что и как нужно писать, а также чего ожидать на ревью.
Тем не менее со Style Guide-ом была большая проблема — люди часто забывали про него, его приходилось постоянно перечитывать и линковать в пулл реквест для доказательства своей правоты. В результате на ревью иногда скатывалось в необъективные споры, получалось что-то такое:
Сами понимаете, как сильно это демотивирует разработчика, и сколько времени уходит на бесполезные споры в ревью. Как результат люди не хотели проходить ревью и ревьюить других разработчиков.
Чтобы голова автора кода и ревьювера меньше была занята вопросами как правильно расставлять запятые и в каком же порядке писать правила для border
мы решили внедрить автоматику, на тот момент это был jshint, стало сильно лучше. После перешли на eslint из-за ряда преимуществ:
- Он более гибкий
- Под него есть всевозможные плагины
- У различных компаний есть хорошие готовые конфиги
Долгое время мы наследовались от конфига airbnb
, но нас в нем не все устраивало, приходилось переопределять некоторые правила. Это было не очень удобно, в результате мы написали свой конфиг на основе airbnb-шного. Так же мы добавили прекомит хуки, на тот момент мы использовали husky. Если разработчик что-то написал не так, он сразу это узнавал, при попытке закоммитить свои изменения в github.
Но к нашему сожалению eslint не покрывал всех моментов по форматированию кода, для решения этой проблемы добавили prettier.
Благо eslint и prettier хорошо работают вместе, надо всего-лишь поставить eslint-plugin-prettier и eslint-config-prettier а после этого поправить .eslintrc
примерно так:
...
"plugins": ["prettier"],
"extends": ["@hh.ru/eslint-config", "prettier"],
"rules": {
"prettier/prettier": ["error"],
...
Перед тем как все это выпускать на прод, надо было пройтись по всей кодовой базе и поправить ее, чтобы она соответствовала новым правилам, это оказалось сделать совсем не сложно: yarn eslint --fix <path_to_js>
После этого большая часть споров про то как писать и форматировать код пропали, так как все покрывается автоматикой. Итого сейчас у нас:
- Весь js и jsx проверяется и автофиксится с помощью eslint и prettier.
- Для less используется stylelint.
- Для python flake8.
Измененные файлы проверяются на машине у разработчика при коммите, при помощи lint-staged, вот наш .lintstagedrc
:
{
"*.{js,jsx}": ["eslint --fix", "git add"],
"*.less": "stylelint",
"*.py": "flake8",
"package.json": "bash -c 'yarn check-versions && yarn install --frozen-lockfile'",
}
В гитхаб попадает код, который уже прошел линтинг и тесты, ревьюверу не нужно больше думать об этом и обращать внимание на мелочи. Можно полностью сосредоточиться над том, насколько хорошо написан код, не будет ли каких проблем с производительностью, подумать про архитектуру.
После ревью собирается докер контейнер, во время сборки прогоняются все автотесты и линтеры. Сейчас весь процесс сборки занимает порядка 7.5 минут, при том, что сейчас у нас порядка 1000 js и 650 less файлов. Все это необходимо, так как локально на машине можно пропустить с помощью --no-verify
, а комментарии в гитхабе не блокируют задачу. Обмануть сборку никак не выйдет.
После прохождения автотестов начинается ручное тестирование. Если тестировщик не находит ни одного бага, задача выходит на прод.
Если на любом из этапов происходит ошибка — задача возвращается на доработку.
В результате стало:
- Проще и быстрее писать код
- Легче делать ревью
- В мастере всегда качественный код
- Меньше споров, больше счастья
Мы следим за качеством кода во время выполнения бизнес задач, но продукт постоянно развивается, в коде появляются дополнительные условия, сложность возрастает. Также появляются новые технологии, старые отмирают из-за всего этого появляется технический налог. В рамках налога мы занимаемся:
- Оптимизацией пользовательских страниц
- Инфраструктурными изменениями
- Улучшением кодовой базы
Все продуктовые команды должны тратить 70% времени на разработку бизнес задач, а 30% на налог — это дает возможность каждому разработчику заниматься инфраструктурными задачами, поддерживать кодовую базу в отличном состоянии, распространять знания про инфраструктуру проекта по всем командам. В качестве налога не обязательно делать задачи своей команды, можно писать код в любую часть проекта. Любой может предложить налог, если он покажется полезным, то его добавят в бэклог. Помимо этого есть архитектурные команды, которые занимаются технологическими фичами все время. Все это позволяет поддерживать кодовую базу в актуальном состоянии.
Комментарии (24)
Free_ze
04.02.2019 18:05+1По моему скромному мнению, настройка линтера в этом случае — это попытка руководства замести под ковер некоторые проблемы в коллективе. Сложно представить, чтобы зануда со скриншота действительно испытывал сложности с чтением свойств с другим порядком аргументов, ради которых стоило бы разворачивать PR.
tapo4ki Автор
04.02.2019 18:20Да это адский пример и такого почти никогда не было. Это скорее к тому что делайте линтинг и не будет у вас такого треша.
JustDont
04.02.2019 18:22Проблема «докопаться до запятых» не обязательно имеет отношение к проблемам в коллективе. Это очень распространённые мозговые тараканы. Bike shed problem — это, собственно, то же самое. Чем менее важен и чем более тривиален какой-то случай — тем больше людей считает возможным высказать своё очень ценное мнение по этому случаю (это не значит, что они будут его высказывать — но само наличие возможности уже означает, что таких случаев будет больше, чем случаев критики реально сложных вещей).
Все эти случаи легко решаются диктатурой сверху: сказали, что будем делать так, все теперь могут замолкнуть и делать. Линтер — это просто технический инструмент обеспечения диктатуры.tapo4ki Автор
04.02.2019 18:39Данный случай это скорее гротеск. По поводу критики неважных вещей согласен. Где-то была старая шутка что-то в стиле «пр +3000 — 2 комента, пр +12 — 11 коментов». Диктатура часто хорошо, чтобы люди не холиварили по поводу того нужно ли ставить `;` или ASI все сам разрулит. Все пишут так как говорит линтер — все не ссорятся. Если какие-то правила не нравятся по объективным причинам можно их и глобально поменять.
Free_ze
04.02.2019 19:01Это очень распространённые мозговые тараканы.
Дихлофосить их надо, а не подкармливать)
С линтером будут холиварить на тему подходов, паттернов, инструментов, кто какую тасочку делает и куда в коде лезет. Дело не в запятых как таковых. Особенности характера)tapo4ki Автор
05.02.2019 10:08Ну так холиварить на тему подходов и паттернов куда как лучше. Это несет какую-то смысловкую нагрузку.
Vadem
04.02.2019 21:29Спасибо за статью, но, если честно, как-то уж слишком кратко.
Код-ревью, линтеры, автотесты и возврат технического долга — вещи само собой разумеющиеся в 2019 году.
Хотелось бы больше подробностей.
Наверняка же есть что-то ещё. Особенно в области тестирования.
Как, например, боритесь c «flaky» тестами при тестировании скриншотами?tapo4ki Автор
05.02.2019 10:13Мы не тестируем скриншотами. Здорово если для вас это само собой разумеющиееся. Не для всех к сожалению это так. В веб студиях до сих пор пилят код, который должен быть готов вчера.
serf
05.02.2019 10:30Поддержу. Все как-то банально, у большинства компаний подобный минимум соблюден, бывает даже опенсорсеры одиночки ведут свои проекты не хуже.
Tabasov
05.02.2019 08:381k файлов, сколько у вас в штате фронтовиков, почему так много файлов?:)
tapo4ki Автор
05.02.2019 09:51У нас очень большой проект. Фронтов больше 20. Большая часть функционала доступна только для работодателей. + сейчас внедряем реакт, там куча файлов.
serf
05.02.2019 10:28Мне кажется это был зарказм тк 1к файлов это немного как по мне, тем более при 20 фронтендеров.
Tabasov
05.02.2019 11:53Нет не сарказм, просто не доводилось работать с такими проектами где по фронту может быть столько логики
serf
05.02.2019 11:57Современные фреймворки пропагандируют компонентный подход к разработке (что хорошо), и это влияет на количество файлов в проекте. Классический компонент это 3 файла (js/ts + html + css/sass) и сами компоненеты могут часто быть совсем небольшими и без особой логики (dump которые, просто отрисовка допустим из центрального redux-like стора).
PS 1к файлов не проблема сгенерировать двум толковым фронтендерам за год.
SayLovePlz
05.02.2019 18:56Почему с husky перешли на lint-staged?
tapo4ki Автор
06.02.2019 10:29Они не заменяют друг друга же. Надо чтобы что-то запустило линт стейдж. Мы решили что нам не нужна лишняя зависимость. Сами на пост инсталле пишем нужный хуки в `.git/hooks/`
Koneru
Налог — это технический долг, только наоборот?
tapo4ki Автор
Примерно так.