Pull to refresh

Comments 27

А мне вот интересно, насколько вообще ревью кода полезная вещь?
Я работал в двух конторах, где оно было внедрено на обязательном уровне, плюс еще в паре мест его пытались внедрить, но там оно быстро глохло.

В первой я работал еще джуниором, и вот там оно мне было реально ползено — фидбек от опытных спецов помогал мне расти + защищал кодовую базу от моего неумелого говнокода.

Во второй, много лет спустя, в команде были уже опытные спецы, и вот тут-то от него было больше вреда чем пользы. За год работы там я в чужом коде смог на ревью найти одну багу-опечатку при копипасте (что наверное проще выявить хорошим статическим анализом кода) и один раз предложил некое архитектурное исправление. Примерно то же самое было и в отношение моего кода. Т.е. какой-то реальной пользы от него было мало, все и так писали неплохой код. При этом в тривиальном коде обычно нет ошибок, а нетривиальный слишком сложен чтобы его сходу прочитать, понять и что-то там найти, в итоге на ревью быстренько формально проглядывали по верхам, не вдаваясь в подробности т.к. и так работы много.

Зато постоянно попадались затыки: человек (особенно если это тимлид) оказывается слишком занят и куча пулл-реквестов висит у него на ревью долгое время, не давая вмерджить их и выкатить фичу, тормозя весь процесс.

В общем я на это все посмотрел, и там где я сам являюсь главным ревью внедряю только для джуниоров, для всех остальных по желанию.
Это у вас какой-то странный случай. У нас на ревью непрерывно что-то интересное находится. Причём количество вопросов по ревью никак не соотносится с уровнем разработчика. Сениор это тоже человек, который может не выспаться, просмотреть описку, забыть поправить копи пейст, написать в горячке неоптимальный код, забыть про то, что происходит в параллельных задачах, и так далее. А цена его ошибки обычно выше, чем у джуниора.

К слову, одна из вещей, которые я, как техлид, не пропускаю на ревью — это «нетривиальный код». Если мне надо думать, что и как делает какой-то код, то там почти наверняка что-то не так.
Если мне надо думать, что и как делает какой-то код, то там почти наверняка что-то не так.

Вот никогда не понимал такого подхода, так же как и смежного понятия «самодокументирующийся код». В какой-нибудь компании-интеграторе такой подход оправдан, так как там практически весь код — это перекладывание объектов из одного апи в другое, из одной коллекции в другую, и что-то нетривиальное действительно встретить сложно. Сложные решения там могут быть на уровне архитектуры, но каждый конкретный кусок кода довольно прост. Но и в таком случае код-ревью может быть затруднено, например когда человек выкатил какое-то крупное изменение, над которым работал последние пару недель — десятки файлов, сотни правок в разных местах, его даже прочитать по верхам сложно и долго, а уж понять и осознать, особенно не владея контекстом задачи которая делалась — слишком много времени тратить.

Ну и не везде код исчерпывается простыми операциями. Сложным может быть код, который считает какие-нибудь хитрые математические вычисления (причем хитрыми могут быть как сами формулы, так и тот извилистый путь которым к ним пришли), или реализует какой-нибудь небанальный алгоритм о котором человек раньше не слышал (вот мне тут понадобилось написать определение зоны видимости ребра полигона на плоскости, если видимость точки сделать тривиально, то видимость ребер уже нет, надо статьи на эту тему почитать). Но его тоже приходится ревьювить. И либо ревьювер тратит кучу времени на вникание в тему (вместо выполнения своих прочих обязанностей), либо ревью оказывается чисто формальным.
Крупных изменений нужно стараться избегать. Или, по крайней мере, разносить изменения по разным коммитам, и комментировать в пулл реквесте, на что нужно или нет обращать внимание (регулярно так делаю со своими пулл реквестами). Ревью одной задачи у меня обычно занимает от 5 до 20 минут, обычно делаю за кофе.
А ещё ревью занимаюсь не только я, но и сениоры команды — это помогает не допускать ситуаций, когда что-то «виснет» на ревью, да и вообще не закопаться насмерть.
Насчёт глубины погружения в код на ревью — тут нужен баланс. С одной стороны, нельзя позволить себе закопаться, с другой — ревью должно приносить больше пользы, чем статический анализатор. У нас получается — часто просто засчёт того, что свежий взгляд подмечает проблемы, которых не видит автор.
Вот никогда не понимал такого подхода, так же как и смежного понятия «самодокументирующийся код».

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

ИМХО, по крайней мере, надо написать так, чтобы


  • читатель смог легко соотнести алгоритм и статью
  • читатель легко понял, что алгоритм делает в целом (что определяет именно видимость и именно полигона, чтобы понял что является входом и что выходом и так далее)
  • читатель мог легко прочитать тесты и понять какие ситуации в них рассмотрены и почему
Во-певых: отвечая на ваш первый коментарий, код ревью не предназначен для поисках багов или тривиальных ошибок и вы абсолютно правы, статические анализаторы и тесты справятся с этим гораздо лучше. Код ревью больше решает проблему совместного владения кодом в команде. При внедренном и работающем процессе код ревью не получится кого либо блеймить за «некрасивы» код, потому что вы его пропустили на код ревью, это очень соответсвует коллективной ответственности в скрам.

Во-вторых: имхо, код может быть «самодокументирующийся» независимо от сложности формул, если вычисления спрятаны за правильными абстракциями.

Хороший вопрос. У меня есть отдельная статья про «правильное» ревью и «неправильное» ревью, где я описываю распространённые ошибки. Скоро опубликуем и подискутируем там в комментариях.
Спасибо!

Ревью кода всей ветки (бранчдиффа) инструмент делать не умел

А в чем смысл ревью всей ветки сразу целиком? Слишком лениво разбивать на логические
и завершенные коммиты?

Ну почему же, такой подход может существовать. Но если взять в уравнение скорость, необходимость аккуратного внедрения в текущий процесс, на текущих задачах, учесть размер компании и ее рост, то все становится не так очевидно. Можно заставить 250 инженеров «правильно» оформлять коммиты перед пушем, но зачем? Какая бизнесу от этого польза?

Фактически, вы только что спросили «А зачем пулл реквесты?».

Не совсем. Бранчдифф это почти то же самое что пулл реквест. Но без необходимости создания отдельной сущности. И более динамичный.
Простой пример — в коде ошибка. Идеология пулл-реквестов во-первых откладывает ее обнаружение на более поздний срок. На момент, когда девелопер оформит пулл-реквест и по нему начнутся автоматические проверки (например). А во-вторых, чтобы ошибку исправить надо пулл реквест отменить и создать новый. Это лишнее время.
В случае с бранчдиффами этого ничего не нужно. Для простоты можно считать что бранчдифф это пулл-реквест, но без лишней бюрократии :)

Хмм, забавно, но у нас в битбакете пулл реквесты работают так же, как у вас бранч диффы:

1. Автоматические проверки начинаются при пуше в ветку. Никакой новой сущности придумывать не нужно — есть просто ветка…
2. Для исправления ошибки делается пуш в ту же ветку, и пулл реквесты сами обновляются. Кстати, на гитхабе так же работает. Так что не знаю, откуда у вас взялась проблема с бюрократией и пересозданием пулл реквестов… Может, раньше так было?

Когда мы только начали делать инструмент, сторонние инструменты были другие, да. И с тех пор немало эволюционировали.
А битбакет разве поддерживает self-hosted установку? Гитхаб может, но стоит очень уж дорого.

Ах да, атлассиан. Уже вижу с каким скрипом это будет у нас работать. И лицензия на 500 пользователей под 20к стоит.
За ссылку спасибо.

Да, атласиановские продукты не быстрые, тем более self hosted. Хотя джиру на 200 пользователей мне удавалось разогнать до довольно шустрой работы.

А как насчёт gitlab community edition? Бесплатно, и я его использовал в двух компаниях без всяких проблем. Про то, как там работают пулл реквесты — честно говоря, не помню, но всегда можно допилить.
gitlab, как я уже упоминал выше, мне видится наиболее перспективным сейчас. Ребята большие молодцы, хорошо развивают свой проект.
В то же время, тогда когда мы его пробовали ранее, продукт был сильно сырым.
В gogs и гитлабе реквесты включают все пуши после создания реквеста. Пересоздавать ничего не нужно. А проверки и сборки работают по пушу в ветку, зачем ждать реквеста?
Глянул по диагонали — вроде нет отличий от того, как это в битбакета и гитлабе сейчас. Может, не заметил что-то? Было бы хорошо, кроме исторического обзора (который очень интересный, спасибо), ещё с современными аналогами сравнить.
Да, видимо было бы неплохо сделать обзорную статью со сравнением :) Давайте так: я дам вводную, а после попробую найти время на полный обзор, но не обещаю что время найдется. Это же кучу инсталляций и замеров надо делать опять, причем держа в голове что это для публичной статьи :)
Вводная:
1) Инструмент должен быть self-hosted.
2) Инструмент должен иметь возможность доработки по мере появления запросов на новые фичи.
3) Бесплатный опенсорсный лучше платного проприетарного :)
4) Инструмент должен поддерживать разделение доступа по репозиториям.
5) Общее кол-во репозиториев в системе — чуть менее 250.
6) Самый большой репозиторий содержит ~130 тысяч файлов, весит около 5 гигабайт. История коммитов в нем на текущий момент ~ 670 тысяч коммитов.

Большинство инструментов, которые мы пробовали, начинали фейлить на 6м пункте — загружали историю нашего репозитория по нескольку дней, недель и т.д. И потом безумно тупили. codeisok же не делает никаких внутренних индексов и работает поверх гита. Получается быстро.
Вот тут в комментариях выше, я описал вводную. Эксперименты с Upsource у нас закончились тем, что в течении двух недель инструмент не смог индексировать историю самого большого репозитория. Мы писали в Jet Brains (у нас есть там контакты со времен укрощения teamcity). Нам ничего вразумительного не смогли посоветовать, а только попросили предоставить им этот самый репозиторий, чтобы они на живых данных проверили что там так долго индексируется. Этого мы, разумеется, себе позволить не смогли.

У нас комитов поменьше, но тоже индексировал не быстро, кроме этого занимал очень много места, в остальном понравился.

Я правильно понял, что вы именно идеологию фичебранчей ввели, а не Git? Т.е. Git внедрили как довесок? Не совсем очевидны преимущества Git перед SVN в плане фичебранчей.
Правильно, в первую очередь нам нужно было разделить задачи по веткам. Один из инструментов, который хорошо и удобно это позволяет делать — git.
судя по описанию в продукте происходит только ревью и обсуждение кода.
А мерджите ветки потом другим инструментом, почему?

Изначально в гите встроенных средств для коллективного ревью кода нет. Поэтому пришлось искать что-то подходящее.
В то время как для мержа веток все есть из коробки. git merge нас устраивает чуть более чем полностью.

Sign up to leave a comment.