LINUX.ORG.RU

Нужны ваши мысли по code review

 ,


2

3

Вообщем у меня есть один долгоживущий проект, посвященный теме ревью кода. Что он делает можно увидеть из этого gif

На данный момент, вся логика максимально отдалена от структур проекта, репозиториев, коммитов и всего такого, поэтому можно комментировать любой кусок кода или произвольный текст, что использовалось на практике для «тыкания носом» в выдержку из какого-нибудь лога или YAML-конфига.

Но для современной разработки этого мало, ревью cейчас проводится по большому количеству файлов, с учетом отличий. Поэтому я буду проект серьезно переделывать.

Хотелось бы услышать ваши мысли и пожелания, в первую очередь по интерфейсу - как это должно выглядеть для максимального удобства проведения code review.

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

★★★
Ответ на: комментарий от alex0x08

Беседа все же для другого предназначена, смысл ревью - выдать оценку а не вступать в переписку, нет?

Ну переписку в пяток коментов (сумарно, от всех участников) под «строкой» от 3-х людей я вижу регулярно. Дальше она уже переносится в личные переписки, созвоны, встречи, и т. д.
Часто нельзя просто сказать «это работать не будет и сделай так-то». Вместо этого завязывается целая дискуссия с коллективным поиском оптимального решения.

Мне кажется, в твоем понимание рецензирование кода похоже на проверку тетради учителем в школе. Как по мне, то у них общего мало, так как в Github идет двусторонняя переписка и раунды исправления.

urxvt ★★★★★
()
Ответ на: комментарий от static_lab

потенциально может быть началом диалога между автором кода и ревьюерами.

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

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

alex0x08 ★★★
() автор топика
Ответ на: комментарий от static_lab

Если речь про заказную разработку (вы же ее имеете ввиду?) то там своих приколов хватает. И главный прикол звучит как: а давайте применим новую технологию Х для нового проекта.

Как-то так заезжают в коммерческие проекты все эти Scala/Clojure/F# и прочие Лиспы.

Потому что профессионалам захотелось.

Другой прикол выглядит как: «а давайте все сразу спроектируем», что может дойти до диаграммы классов в UML включая наследование.

Так что с коммерческой разработкой я тоже много всякого натерпелся.

alex0x08 ★★★
() автор топика
Ответ на: комментарий от alex0x08

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

Разговор сейчас не о сраче, а об осуждении задачи. Очень часто, нужно что-то реализовать и только при попытке реализации (на этапе просмотра пулреквеста) люди понимают, что все вообще не так как они это представляли. Соответственно, рождается не срач а попытка найти решение задачи. Ну или даже переосмысления задачи.

urxvt ★★★★★
()
Ответ на: комментарий от static_lab

А если диалог перерастает в срач, то тут чаще всего есть субъективный фактор.

Мало кто умеет связно выражать мысли в тексте, мало кто умеет держать себя в рамках и помнить о смысле: не пообсужать чего-то а достичь завершения задачи или разработки и тд.

Проблема с диалогами в том что они потенциально бесконечны.

alex0x08 ★★★
() автор топика
Ответ на: комментарий от urxvt

Соответственно, рождается не срач а попытка найти решение задачи. Ну или даже переосмысления задачи.

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

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

alex0x08 ★★★
() автор топика
Ответ на: комментарий от alex0x08

Ммм... да везде такое я видел. От галер а-ля ЕПАМ и стартапов и до ФААНГов. Про тот же Гугл такое же пишут: нужно подтверждения от 2-х любых аудиторов.

P. S.
Я не имел ввиду, что ПР смотрят все в обязательном порядке. Я о том, что после создания ПР кто хочет тот его и смотрит. И в среднем получается, что каждый член команды просматривает 50-70% всех ПРов.

urxvt ★★★★★
()
Ответ на: комментарий от alex0x08

Проблема с диалогами в том что они потенциально бесконечны.

Если в команде в ПРах идет война в духе loops vs streams то это плохие новости. Обычно бесконечность диалогов пресекается простым правилом: если в диалоге больше чем N сообщений то его нужно перенести на встречу (например ежедневные созвоны, или как там у вас коллеги общаются). Все. Problem solved.

urxvt ★★★★★
()
Ответ на: комментарий от alex0x08

Мало кто умеет связно выражать мысли в тексте

Кодинг или написание техдокументации тоже требуют умения выражать мысли в тексте

мало кто умеет держать себя в рамках и помнить о смысле

Такие люди могут и в программировании вместо решения конкретной задачи начать реализовывать абстракции над абстракциями

Проблема с диалогами в том что они потенциально бесконечны

Вы же не боитесь вступать на ЛОРе в диалоги

static_lab ★★★★★
()
Ответ на: комментарий от urxvt

Я о том, что после создания ПР кто хочет тот его и смотрит. И в среднем получается, что каждый член команды просматривает 50-70% всех ПРов.

Не очень понимаю такую вольницу, поскольку это работает пока нет завала по своей работе. Как только наступает аврал со своими задачами - все эти «посмотреть PR» идут лесом.

alex0x08 ★★★
() автор топика
Ответ на: комментарий от alex0x08

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

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

urxvt ★★★★★
()
Ответ на: комментарий от alex0x08

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

По опыту, если это действительно требуется, то как раз и не нужно описывать всё в коммите, а инициировать обсуждение. Но без начальной переписки потребность переосмысления задачи или решения не всегда очевидна.

static_lab ★★★★★
()
Ответ на: комментарий от alex0x08

Согласен, это работает не всегда. В аврале аудит не работает вообще, с таким тоже сталкивался. Любой не работает, хоть вольница, хоть невольница. Иначе, как уже упоминали, работа бригадира будет на 80% состоять из просмотра ПРов.

urxvt ★★★★★
()
Ответ на: комментарий от alex0x08

это работает пока нет завала по своей работе

Но это ведь тоже провал менеджмента. Если обязанность ревьюить возложена исключительно на руководителя, то у него тоже может образоваться завал по другим его задачам.

static_lab ★★★★★
()
Ответ на: комментарий от urxvt

Да не в войне дело, обычно это бесконечное уточнение деталей: «повторилось-не повторилось», уточнение каких-то пограничных состояний при которых «работает но не так». И эта петрушка очень легко затягивается на месяцы.

Это видно по багтрекеру Wine или Убунты, где обычные пользователи репортят и участвуют в переписке.

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

alex0x08 ★★★
() автор топика
Ответ на: комментарий от alex0x08

уточнение каких-то пограничных состояний при которых «работает но не так»

а это не нужно вносить в тесты?

Это видно по багтрекеру Wine или Убунты, где обычные пользователи репортят и участвуют в переписке.

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

static_lab ★★★★★
()
Ответ на: комментарий от static_lab

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

Поэтому вполне может быть что:

обязанность ревьюить возложена исключительно на руководителя,

но проект при этом уже стабилен и работает.

alex0x08 ★★★
() автор топика
Ответ на: комментарий от alex0x08

Абсолютно согласен. Для этого есть условная Jira, в которой можно фиксировать результаты тестирования. В любом случае, это лучше чем email, так как они между собой связаны и доступны всем нужным людям в компании. И людям которые придут через год в компанию.

urxvt ★★★★★
()
Ответ на: комментарий от static_lab

а это не нужно вносить в тесты?

Не все поддается автоматизированному тестированию. Тестировать автотестами поехавшую верстку на разных браузерах может себе позволить только гугол с яндексом, ну или ребята их размеров.

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

Ближе к QA по духу ) Но я про другое - про то как и почему переписка превращается в бесконечную.

alex0x08 ★★★
() автор топика
Ответ на: комментарий от alex0x08

Тестировать автотестами поехавшую верстку на разных браузерах может себе позволить только гугол с яндексом, ну или ребята их размеров.

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

static_lab ★★★★★
()
Ответ на: комментарий от urxvt

Для этого есть условная Jira

Я этим летом внезапно ошарашил директора одной ИТ-компании вопросом про бекапы их трекера. Джира настолько стабильно и хорошо работает годами, что люди просто забывают про ее устройство и необходимость делать бекап. В этом случае речь шла о где-то 50 разных проектах, которые велись в одной Джире без каких-либо бекапов в течение трех лет. Что бы случилось в случае порчи базы данных думаю можно легко представить.

И людям которые придут через год в компанию.

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

alex0x08 ★★★
() автор топика
Ответ на: комментарий от static_lab

то некритичный баг можно договориться вынести в отдельную задачу.

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

alex0x08 ★★★
() автор топика
Ответ на: комментарий от alex0x08

У нас чуть ли не раз в неделю такое, что перекинулись парой сообщений в PR и завели на основе этого новую задачу.

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

urxvt ★★★★★
()
Ответ на: комментарий от urxvt

автор сам добавляет комментарии к некоторым строкам кода

Потом это мёржится, код начинает жить отдельно от этого мёрж-реквеста, реквест тонет в Лете, новые программисты находят тот код уже без комментариев и дивятся на него, как на артефакт внеземных цивилизаций.

Мне такое очень нравится, да. Имел счастье наблюдать вживую. ☺️

aol ★★★★★
()
Ответ на: комментарий от urxvt

Так вот, после создания PR автор сам добавляет комментарии к некоторым строкам кода, если желает объяснить принятые им решения. Мне такой подход очень понравился, сильно упрощает аудит.

Идея не очень, поскольку эти комментарии очень легко потерять при рефакторинге. Они же хранятся отдельно от кода и привязаны к конкретным строчкам в файле.

Лучше описывать комментариями в самом коде такое.

alex0x08 ★★★
() автор топика
Ответ на: комментарий от alysnix

архитекторы кодингом не занимаются.

Каждый божий день. И чтение чужого и написание своего, пусть не в таком объеме как программисты.

По поводу code review, если команда синьоров, то договорившись на берегу дальше проблем особо нет.

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

Если команда смешана, то миддлы назначается на первое время синьорам и уже они проверяют коммиты мидла.

Ну и sublime merge, git blame регулярно.

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

Obezyan
()
Ответ на: комментарий от Obezyan

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

Это вообще-то называется хорошей практикой, поскольку дает защиту «от дурака», сложно представить большой проект без всех этих мер, пусть даже команда состоит из одних звезд разработки.

Большой проект без таких средств это примерно как пистолет без предохранителя: использовать можно, но с постоянным риском отстрелить себе яйца в самый неожиданный момент.

alex0x08 ★★★
() автор топика
Ответ на: комментарий от static_lab

Ну да ну да, будут мержи на основании истории миллиона мержей, половина из которых были проблемными. Проще уж тогда монетку подбрасывать на удачу.

alex0x08 ★★★
() автор топика
Ответ на: комментарий от alex0x08

Это вообще-то называется хорошей практикой

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

А линтер нужен не всегда и не везде, как и постоянный code review каждого коммита.

Obezyan
()
Ответ на: комментарий от alex0x08

Про конфликты пришел к такому наблюдению: частые конфликты при мерджах являются индикатором того что что-то идет не так.

Например, коммиты лежат и ждут своего часа до мерджа неделями, иногда - месяцами, при достаточно активной разработке (косяк в методологии работы).

Или когда много разработчиков работают над разными задачами в одном и том же месте в коде (плохая изоляция сущностей - косяк в архитектуре).

Понятно что не все и не всегда можно пофиксить правильной архитектурой и разделением задач, но как-то получается все свести до 1-2 конфликтов в месяц и меньше.

Obezyan
()
Ответ на: комментарий от alex0x08

Большинство людей адекватны и аргументы вполне воспринимают. С неадекватами работать - себе дороже. Если не можешь аргументированно зашеймить код человека то проблема на твоей стороне.

ya-betmen ★★★★★
()
Ответ на: комментарий от urxvt

аудитор - это работа такая. проверять качество и ставить штампик своих гарантий.

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

что будет, если два гуру будут аудировать друг друга… во-первых гуру кодом уже наелись, и чужой код - только глазки портить, а во вторых они уверены в коде друг друга и его качестве.

за описками должен смотреть компилятор. форматирование кода надо выучить из текущего кодестайла.

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

alysnix ★★★
()
Ответ на: комментарий от Obezyan

Я имел ввиду что в зависимости от качества команды

За редкими исключениями вроде R&D-отделов, качество команды разработки будет примерно одинаковым, что в Гугле что в каком-нибудь Люксофте.

Никаких ресурсов не хватит собрать в одной команде 50 суперзвезд, поэтому будет звезда одна, еще десять - болтаться у нее на орбите, а основная масса будут откровенными середнячками.

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

Отдельно спецназ, отдельно армия.

alex0x08 ★★★
() автор топика