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)


  1. Koneru
    04.02.2019 17:01

    Налог — это технический долг, только наоборот?


    1. tapo4ki Автор
      04.02.2019 18:17

      Примерно так.


  1. Free_ze
    04.02.2019 18:05
    +1

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


    1. tapo4ki Автор
      04.02.2019 18:20

      Да это адский пример и такого почти никогда не было. Это скорее к тому что делайте линтинг и не будет у вас такого треша.


    1. JustDont
      04.02.2019 18:22

      Проблема «докопаться до запятых» не обязательно имеет отношение к проблемам в коллективе. Это очень распространённые мозговые тараканы. Bike shed problem — это, собственно, то же самое. Чем менее важен и чем более тривиален какой-то случай — тем больше людей считает возможным высказать своё очень ценное мнение по этому случаю (это не значит, что они будут его высказывать — но само наличие возможности уже означает, что таких случаев будет больше, чем случаев критики реально сложных вещей).

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


      1. tapo4ki Автор
        04.02.2019 18:39

        Данный случай это скорее гротеск. По поводу критики неважных вещей согласен. Где-то была старая шутка что-то в стиле «пр +3000 — 2 комента, пр +12 — 11 коментов». Диктатура часто хорошо, чтобы люди не холиварили по поводу того нужно ли ставить `;` или ASI все сам разрулит. Все пишут так как говорит линтер — все не ссорятся. Если какие-то правила не нравятся по объективным причинам можно их и глобально поменять.


      1. Free_ze
        04.02.2019 19:01

        Это очень распространённые мозговые тараканы.

        Дихлофосить их надо, а не подкармливать)

        С линтером будут холиварить на тему подходов, паттернов, инструментов, кто какую тасочку делает и куда в коде лезет. Дело не в запятых как таковых. Особенности характера)


        1. tapo4ki Автор
          05.02.2019 10:08

          Ну так холиварить на тему подходов и паттернов куда как лучше. Это несет какую-то смысловкую нагрузку.


  1. Vadem
    04.02.2019 21:29

    Спасибо за статью, но, если честно, как-то уж слишком кратко.
    Код-ревью, линтеры, автотесты и возврат технического долга — вещи само собой разумеющиеся в 2019 году.
    Хотелось бы больше подробностей.
    Наверняка же есть что-то ещё. Особенно в области тестирования.
    Как, например, боритесь c «flaky» тестами при тестировании скриншотами?


    1. tapo4ki Автор
      05.02.2019 10:13

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


    1. serf
      05.02.2019 10:30

      Поддержу. Все как-то банально, у большинства компаний подобный минимум соблюден, бывает даже опенсорсеры одиночки ведут свои проекты не хуже.


  1. Tabasov
    05.02.2019 08:38

    1k файлов, сколько у вас в штате фронтовиков, почему так много файлов?:)


    1. tapo4ki Автор
      05.02.2019 09:51

      У нас очень большой проект. Фронтов больше 20. Большая часть функционала доступна только для работодателей. + сейчас внедряем реакт, там куча файлов.


      1. serf
        05.02.2019 10:28

        Мне кажется это был зарказм тк 1к файлов это немного как по мне, тем более при 20 фронтендеров.


        1. Tabasov
          05.02.2019 11:53

          Нет не сарказм, просто не доводилось работать с такими проектами где по фронту может быть столько логики


          1. serf
            05.02.2019 11:57

            Современные фреймворки пропагандируют компонентный подход к разработке (что хорошо), и это влияет на количество файлов в проекте. Классический компонент это 3 файла (js/ts + html + css/sass) и сами компоненеты могут часто быть совсем небольшими и без особой логики (dump которые, просто отрисовка допустим из центрального redux-like стора).

            PS 1к файлов не проблема сгенерировать двум толковым фронтендерам за год.


  1. RuGrof
    05.02.2019 09:36

    Где-то вы должны дойти до prettier :)
    Чтоб таких ситуаций было ещё меньше.


    1. tapo4ki Автор
      05.02.2019 09:51

      Так дошли же.


      1. RuGrof
        05.02.2019 11:50

        Точно, что-то упустил этот момент.


  1. EskakDolar
    05.02.2019 10:20

    Пишите лучше hh. А то НН можно за Нижний Новгород принять.


  1. worldxaker
    05.02.2019 12:23

    а типизацию используете?


    1. tapo4ki Автор
      05.02.2019 12:26

      Увы, пока нет. Думаем над этим вопросом.


  1. SayLovePlz
    05.02.2019 18:56

    Почему с husky перешли на lint-staged?


    1. tapo4ki Автор
      06.02.2019 10:29

      Они не заменяют друг друга же. Надо чтобы что-то запустило линт стейдж. Мы решили что нам не нужна лишняя зависимость. Сами на пост инсталле пишем нужный хуки в `.git/hooks/`