Comments 27
Я работал в двух конторах, где оно было внедрено на обязательном уровне, плюс еще в паре мест его пытались внедрить, но там оно быстро глохло.
В первой я работал еще джуниором, и вот там оно мне было реально ползено — фидбек от опытных спецов помогал мне расти + защищал кодовую базу от моего неумелого говнокода.
Во второй, много лет спустя, в команде были уже опытные спецы, и вот тут-то от него было больше вреда чем пользы. За год работы там я в чужом коде смог на ревью найти одну багу-опечатку при копипасте (что наверное проще выявить хорошим статическим анализом кода) и один раз предложил некое архитектурное исправление. Примерно то же самое было и в отношение моего кода. Т.е. какой-то реальной пользы от него было мало, все и так писали неплохой код. При этом в тривиальном коде обычно нет ошибок, а нетривиальный слишком сложен чтобы его сходу прочитать, понять и что-то там найти, в итоге на ревью быстренько формально проглядывали по верхам, не вдаваясь в подробности т.к. и так работы много.
Зато постоянно попадались затыки: человек (особенно если это тимлид) оказывается слишком занят и куча пулл-реквестов висит у него на ревью долгое время, не давая вмерджить их и выкатить фичу, тормозя весь процесс.
В общем я на это все посмотрел, и там где я сам являюсь главным ревью внедряю только для джуниоров, для всех остальных по желанию.
К слову, одна из вещей, которые я, как техлид, не пропускаю на ревью — это «нетривиальный код». Если мне надо думать, что и как делает какой-то код, то там почти наверняка что-то не так.
Если мне надо думать, что и как делает какой-то код, то там почти наверняка что-то не так.
Вот никогда не понимал такого подхода, так же как и смежного понятия «самодокументирующийся код». В какой-нибудь компании-интеграторе такой подход оправдан, так как там практически весь код — это перекладывание объектов из одного апи в другое, из одной коллекции в другую, и что-то нетривиальное действительно встретить сложно. Сложные решения там могут быть на уровне архитектуры, но каждый конкретный кусок кода довольно прост. Но и в таком случае код-ревью может быть затруднено, например когда человек выкатил какое-то крупное изменение, над которым работал последние пару недель — десятки файлов, сотни правок в разных местах, его даже прочитать по верхам сложно и долго, а уж понять и осознать, особенно не владея контекстом задачи которая делалась — слишком много времени тратить.
Ну и не везде код исчерпывается простыми операциями. Сложным может быть код, который считает какие-нибудь хитрые математические вычисления (причем хитрыми могут быть как сами формулы, так и тот извилистый путь которым к ним пришли), или реализует какой-нибудь небанальный алгоритм о котором человек раньше не слышал (вот мне тут понадобилось написать определение зоны видимости ребра полигона на плоскости, если видимость точки сделать тривиально, то видимость ребер уже нет, надо статьи на эту тему почитать). Но его тоже приходится ревьювить. И либо ревьювер тратит кучу времени на вникание в тему (вместо выполнения своих прочих обязанностей), либо ревью оказывается чисто формальным.
А ещё ревью занимаюсь не только я, но и сениоры команды — это помогает не допускать ситуаций, когда что-то «виснет» на ревью, да и вообще не закопаться насмерть.
Насчёт глубины погружения в код на ревью — тут нужен баланс. С одной стороны, нельзя позволить себе закопаться, с другой — ревью должно приносить больше пользы, чем статический анализатор. У нас получается — часто просто засчёт того, что свежий взгляд подмечает проблемы, которых не видит автор.
Вот никогда не понимал такого подхода, так же как и смежного понятия «самодокументирующийся код».
вот мне тут понадобилось написать определение зоны видимости ребра полигона на плоскости, если видимость точки сделать тривиально, то видимость ребер уже нет, надо статьи на эту тему почитать
ИМХО, по крайней мере, надо написать так, чтобы
- читатель смог легко соотнести алгоритм и статью
- читатель легко понял, что алгоритм делает в целом (что определяет именно видимость и именно полигона, чтобы понял что является входом и что выходом и так далее)
- читатель мог легко прочитать тесты и понять какие ситуации в них рассмотрены и почему
Во-вторых: имхо, код может быть «самодокументирующийся» независимо от сложности формул, если вычисления спрятаны за правильными абстракциями.
Хороший вопрос. У меня есть отдельная статья про «правильное» ревью и «неправильное» ревью, где я описываю распространённые ошибки. Скоро опубликуем и подискутируем там в комментариях.
Спасибо!
Ревью кода всей ветки (бранчдиффа) инструмент делать не умел
А в чем смысл ревью всей ветки сразу целиком? Слишком лениво разбивать на логические
и завершенные коммиты?
Ну почему же, такой подход может существовать. Но если взять в уравнение скорость, необходимость аккуратного внедрения в текущий процесс, на текущих задачах, учесть размер компании и ее рост, то все становится не так очевидно. Можно заставить 250 инженеров «правильно» оформлять коммиты перед пушем, но зачем? Какая бизнесу от этого польза?
Не совсем. Бранчдифф это почти то же самое что пулл реквест. Но без необходимости создания отдельной сущности. И более динамичный.
Простой пример — в коде ошибка. Идеология пулл-реквестов во-первых откладывает ее обнаружение на более поздний срок. На момент, когда девелопер оформит пулл-реквест и по нему начнутся автоматические проверки (например). А во-вторых, чтобы ошибку исправить надо пулл реквест отменить и создать новый. Это лишнее время.
В случае с бранчдиффами этого ничего не нужно. Для простоты можно считать что бранчдифф это пулл-реквест, но без лишней бюрократии :)
1. Автоматические проверки начинаются при пуше в ветку. Никакой новой сущности придумывать не нужно — есть просто ветка…
2. Для исправления ошибки делается пуш в ту же ветку, и пулл реквесты сами обновляются. Кстати, на гитхабе так же работает. Так что не знаю, откуда у вас взялась проблема с бюрократией и пересозданием пулл реквестов… Может, раньше так было?
Когда мы только начали делать инструмент, сторонние инструменты были другие, да. И с тех пор немало эволюционировали.
А битбакет разве поддерживает self-hosted установку? Гитхаб может, но стоит очень уж дорого.
Конечно, ведь он прямо так и называется Bitbucket Server https://www.atlassian.com/software/bitbucket/server
Ах да, атлассиан. Уже вижу с каким скрипом это будет у нас работать. И лицензия на 500 пользователей под 20к стоит.
За ссылку спасибо.
А как насчёт gitlab community edition? Бесплатно, и я его использовал в двух компаниях без всяких проблем. Про то, как там работают пулл реквесты — честно говоря, не помню, но всегда можно допилить.
Вводная:
1) Инструмент должен быть self-hosted.
2) Инструмент должен иметь возможность доработки по мере появления запросов на новые фичи.
3) Бесплатный опенсорсный лучше платного проприетарного :)
4) Инструмент должен поддерживать разделение доступа по репозиториям.
5) Общее кол-во репозиториев в системе — чуть менее 250.
6) Самый большой репозиторий содержит ~130 тысяч файлов, весит около 5 гигабайт. История коммитов в нем на текущий момент ~ 670 тысяч коммитов.
Большинство инструментов, которые мы пробовали, начинали фейлить на 6м пункте — загружали историю нашего репозитория по нескольку дней, недель и т.д. И потом безумно тупили. codeisok же не делает никаких внутренних индексов и работает поверх гита. Получается быстро.
А чем Upsource не понравился?
А мерджите ветки потом другим инструментом, почему?
Изначально в гите встроенных средств для коллективного ревью кода нет. Поэтому пришлось искать что-то подходящее.
В то время как для мержа веток все есть из коробки. git merge нас устраивает чуть более чем полностью.
Codeisok, или История code review в Badoo