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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

Звучит уж слишком хардкорно. Это какой-то масонский орден?

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

Может быть, а может быть и нет. Зависит от проекта и команды.

Хотелось бы услышать ваши мысли и пожелания, в первую очередь по интерфейсу

Нужны реакции на комментарии, как на ЛОРе.

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

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

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

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

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

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

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

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

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

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

Бекапы это классика, да. :) Но это уже совершенно другая задача.

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

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

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

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

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

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

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

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

Идея для пулреквест-тулзы: формировать на основе комментария в тулзе комментарий в коде.

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

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

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

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

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

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

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

И чтоб все конфликты сами резолвились. И чтоб за пивом бегало )

И чтоб все конфликты сами резолвились

Copilot

И чтоб за пивом бегало

Робот-курьер Яндекса

И чтоб все конфликты сами резолвились

Copilot

Вот почему-то нейросеть резолвить конфликты в коде до сих пор не умеет.

Есть куда расти, очевидно же. Может быть в недрах Гитхаба или Майкрософта что-то такое уже пытаются сделать.

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

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

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

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

Ты просто не работай с такими.

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

Кого дают с тем и работаю. Привередливость в таких вопросах сильно дорого стоит.

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

Я это и пытался сказать )

Code review для всей армии не получится, а для спецназа нужен довольно редко.

к тегу «todo» взять да и придумать тег «wtf?»