Code Review

[Херовимчик]

А давайте поговорим о безопасных (от дураков) и эффективных практиках.

Дано:
- большая code base
- 40-50 контрибьютеров (почти десяток команд, каждая со своими тараканами и традициями)

Есть у нас:
1. CLI (авторы одной из команд котрибьютеров). Тул призван к унификации гит коммита, чтобы все запятая к запятой, точка к точке и т.д. Также тул привязан к системе багтрэкинга. Вообщем, все чтобы даже мысли не было создать PR через UI и забыть любую мелочь
2. Core reviewers - люди без чьего благословенная ни один PR нельзя смерджить. Это люди следящие за архитектурой и стилистикой. Их мало, а PR траффик огромный
3. Code owners - либо владелец данного кода, либо SME. Соотношение технологий к экспертам неравномерное, так что тоже людей мало, а PR-ов много. Без их благословения тоже нельзя смерджить.

#3 и #2 это одна из доп обязанностей твоей роли, а ещё работать надо (с). Когда большие изменения, людей нужно преследовать/подкупать чтобы PR не застрял на месяцы
#1 качество такое, что народ набил руку на формат и все равно через UI делает (не, есть такие что неделями будут сидеть и ждать пока починят, остановив всю работу). Но если в тул добавили что-то новое, а ты вручную не отразил это изменение то быстро вычислят и придут стыдить (реально есть отдельные личности с OCD на эту тему)

Все 3 по отдельности очень эффективные и полезные вещи. Вместе - мрак и безнадега. Одни заблокированы, потому что все заняты. У вторых глаз дергается, потому что количество PR-ов на ревью зашкаливает. «А заводы стоят!» (с)

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

А как у вас?

[alex_127]

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

[Херовимчик]

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

Все верно говорите, но в такой большой группе контрибьютеров участники приходят и уходят (даже если взять в среднем по одному интерну и контрактору на каждую команду, то за год это 10-15 новых людей, интерны как раз 3-6 месяцев и тю-тю).
Ну и тут есть две оппозиции (и конечно же не без политических мотивов), где одни въедливые до зубного скрежета, а другие направляют энергию и фокус на более важные вещи. У обоих сторон есть свои плюсы и минусы.

[alex_127]

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

Все верно говорите, но в такой большой группе контрибьютеров участники приходят и уходят (даже если взять в среднем по одному интерну и контрактору на каждую команду, то за год это 10-15 новых людей, интерны как раз 3-6 месяцев и тю-тю).
Ну и тут есть две оппозиции (и конечно же не без политических мотивов), где одни въедливые до зубного скрежета, а другие направляют энергию и фокус на более важные вещи. У обоих сторон есть свои плюсы и минусы.

если честно то я очень не привык к 4-6 месяцам rotation. я не уверен что мог бы сделать хоть что-то.

[Херовимчик]

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

Все верно говорите, но в такой большой группе контрибьютеров участники приходят и уходят (даже если взять в среднем по одному интерну и контрактору на каждую команду, то за год это 10-15 новых людей, интерны как раз 3-6 месяцев и тю-тю).
Ну и тут есть две оппозиции (и конечно же не без политических мотивов), где одни въедливые до зубного скрежета, а другие направляют энергию и фокус на более важные вещи. У обоих сторон есть свои плюсы и минусы.

если честно то я очень не привык к 4-6 месяцам rotation. я не уверен что мог бы сделать хоть что-то.

Rotation тут вообще не причём. Вопрос как защитить код от «временных» контрибьютеров и пофигистов, но не доводить процесс до идиотизма.
В условиях когда архитектура чуть ли не каждый день меняется, идёт постоянный рефакторинг легаси, убирание костылей после миграции, новый девелопмент/баг фиксинг и proactive scalability (все это делают разные группы людей и одновременно).

[Komissar]

забей, мать. Сделайте упор на comprehensive testing, integration testing, а на пробелы / табы и прочие красивости наплюй. У меня за плечами 30 лет успешного выкатывания заказчикам хорошего качественного софта, поверь, code review это неэффективное использование вашего времени, ну за исключением менторинга полных джунов.

[Херовимчик]

забей, мать. Сделайте упор на comprehensive testing, integration testing, а на пробелы / табы и прочие красивости наплюй. У меня за плечами 30 лет успешного выкатывания заказчикам хорошего качественного софта, поверь, code review это неэффективное использование вашего времени, ну за исключением менторинга полных джунов.

После 2+ часов заслушивания оппозиций я забила и адаптировалась (я в лагере разумных пофигистов)… но тема поднимается каждые 2-3 месяца, по уменьшаем контроль, то закручиваем гайки. Оппозиция не сдаётся, и эта холодная война уже достала. Меня угораздило публично высказаться по всем 3-м пунктам, со всеми плюсами и минусами, с реальными примерами последствий за что была поймана с поличным и отправлена в focus group как представитель своей оппозиции. Вообщем, собираю статистику и изучаю существующие практики, а то уже все устали, но всем лень разобраться и поставить точку

(Когда «вражеский» Sr Manager мне первый раз публично ткнул под нос PR-ом не по уставу, я долго думала чего ему вообще надо. Его публично застыдили, так он теперь в личку мне ими тычет спасибо хоть не каждым теперь, а копит кучку сначала)

[M. Ridcully]

забей, мать. Сделайте упор на comprehensive testing, integration testing, а на пробелы / табы и прочие красивости наплюй. У меня за плечами 30 лет успешного выкатывания заказчикам хорошего качественного софта, поверь, code review это неэффективное использование вашего времени, ну за исключением менторинга полных джунов.

-1.
Нельзя на пробелы забивать, но и время на это тратить не надо.

Херовимчик, вы ж, вроде, в Омозоне, или что-то вроде того? А тут детсад какой-то по процессу.
Для запятых и пробелов - добавить в CI соответствующий линтер, и все. Будет, как любой другой тест: поломал - почини. Или необходимость keep CI green тоже кому-то обосновывать надо? Ну и соответствующий тул на стороне инженеров, вроде clang-format.

А code review - это не про стиль, это по-существу должно быть. У нас, кстати, дело сугубо добровольное. У всех есть право пушить прямо в мастер. Но и все тулзы есть для code review - для некоторых изменений делаем gitlab merge requests. Причем иногда по инициативе автора - просто хочется, чтобы кто-то знакомый с соответствующим куском кода второй парой глах глянул. Ну то есть по реальной необходимости, а не как бюрократическое требование.

А вообще, работал в компаниях, где review как требование что-то запушить - тот еще геморрой. Замедляет процесс неимоверно. Workflow запушил-поломал-починил _офигительно_ выигрывает в этом смысле.

[Komissar]

А вообще, работал в компаниях, где review как требование что-то запушить - тот еще геморрой. Замедляет процесс неимоверно. Workflow запушил-поломал-починил _офигительно_ выигрывает в этом смысле.

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

[Херовимчик]

-1.
Нельзя на пробелы забивать, но и время на это тратить не надо.

Херовимчик, вы ж, вроде, в Омозоне, или что-то вроде того? А тут детсад какой-то по процессу.
Для запятых и пробелов - добавить в CI соответствующий линтер, и все. Будет, как любой другой тест: поломал - почини. Или необходимость keep CI green тоже кому-то обосновывать надо? Ну и соответствующий тул на стороне инженеров, вроде clang-format.

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

[mister-X]

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

[M. Ridcully]

-1.
Нельзя на пробелы забивать, но и время на это тратить не надо.

Херовимчик, вы ж, вроде, в Омозоне, или что-то вроде того? А тут детсад какой-то по процессу.
Для запятых и пробелов - добавить в CI соответствующий линтер, и все. Будет, как любой другой тест: поломал - почини. Или необходимость keep CI green тоже кому-то обосновывать надо? Ну и соответствующий тул на стороне инженеров, вроде clang-format.

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

А о каком именно туле на стороне инженеров идет речь - что именно он делает, ну чтобы понять? Этот тул нужен, чтобы как-то "правильно" приготовить коммит? По-моему, это неправильно. Нужно енфорсить не процесс подготовки коммита, а результат. То есть, как выше написал - включить в CI соответствующий линтер, среди прочих тестов.
А чем именно отдельные инженеры будут пользоваться на своих местах - это их дело. Скажем, если если ресь про C++ - то я пользуюсь clang-format. Если кто-то пользует, скажем, CLion - уверен, его тоже можно настроить, как надо. Если кто хочет в ручную форматировать - тоже, ради бога. Линтер потом все проверит.

[Херовимчик]

-1.
Нельзя на пробелы забивать, но и время на это тратить не надо.

Херовимчик, вы ж, вроде, в Омозоне, или что-то вроде того? А тут детсад какой-то по процессу.
Для запятых и пробелов - добавить в CI соответствующий линтер, и все. Будет, как любой другой тест: поломал - почини. Или необходимость keep CI green тоже кому-то обосновывать надо? Ну и соответствующий тул на стороне инженеров, вроде clang-format.

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

А о каком именно туле на стороне инженеров идет речь - что именно он делает, ну чтобы понять? Этот тул нужен, чтобы как-то "правильно" приготовить коммит? По-моему, это неправильно. Нужно енфорсить не процесс подготовки коммита, а результат. То есть, как выше написал - включить в CI соответствующий линтер, среди прочих тестов.
А чем именно отдельные инженеры будут пользоваться на своих местах - это их дело. Скажем, если если ресь про C++ - то я пользуюсь clang-format. Если кто-то пользует, скажем, CLion - уверен, его тоже можно настроить, как надо. Если кто хочет в ручную форматировать - тоже, ради бога. Линтер потом все проверит.

Да в том-то и дело, что они придумали этот доморощенный тул, который правильно подготовит коммит + слинкует PR с тикетом (эта часть удобная). Вот если бы они сами им пользовались и не жужжали, все было бы хорошо. Но они уже даже директора достали
Вообщем, игнорировать или послать не получилось. Переходим к цивилизованной и модной “healthy discussion”.

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

Я полтора года не вступала в этот бой. Но ситуацию довели до абсурда и призвали тяжёлую артиллерию

[Komissar]

Я полтора года не вступала в этот бой. Но ситуацию довели до абсурда и призвали тяжёлую артиллерию

забей. жизнь коротка.

[Херовимчик]

Я полтора года не вступала в этот бой. Но ситуацию довели до абсурда и призвали тяжёлую артиллерию

забей. жизнь коротка.

Нельзя, велено разобраться и прекратить послать так, чтобы наконец-то дошло

[alex_127]

Я полтора года не вступала в этот бой. Но ситуацию довели до абсурда и призвали тяжёлую артиллерию

Я честно не знаю как можно стать эффективным членом команды если средее время нахождения в ней 6 месяцев. Вы знаете? круто. Это не мое, я так не умею. меня не зовите и я вам не подойду.

[Komissar]

Я полтора года не вступала в этот бой. Но ситуацию довели до абсурда и призвали тяжёлую артиллерию

забей. жизнь коротка.

Нельзя, велено разобраться и прекратить послать так, чтобы наконец-то дошло

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

[Херовимчик]

Я полтора года не вступала в этот бой. Но ситуацию довели до абсурда и призвали тяжёлую артиллерию

Я честно не знаю как можно стать эффективным членом команды если средее время нахождения в ней 6 месяцев. Вы знаете? круто. Это не мое, я так не умею. меня не зовите и я вам не подойду.

Я не знаю откуда вы сделали вывод о 6 месяцах? Средний tenure основных участников 5 лет. Контракторы приходят и уходят, из-за них расчитывать на сознательность не приходиться

[quote=Херовимчик post_id=7489210 time=<a href="tel:1627431695">1627431695</a> user_id=49996]
[quote=Komissar post_id=7489200 time=<a href="tel:1627430491">1627430491</a> user_id=8009]
[quote=Херовимчик post_id=7489193 time=<a href="tel:1627427537">1627427537</a> user_id=49996]
Я полтора года не вступала в этот бой. Но ситуацию довели до абсурда и призвали тяжёлую артиллерию

забей. жизнь коротка.
[/quote]

Нельзя, велено разобраться и прекратить послать так, чтобы наконец-то дошло
[/quote]

прекратить... что именно? Поломки билдов, баги в софте или раздоры между группами?
[/quote]

Раздоры

[KVA]

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

Раздоры

Сочувствую. Если нет авторитета или authority то ничего не выйдет.

[alex_127]

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

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

[Херовимчик]

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

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

Вопрос никак уладить раздор. Вопрос о существующих практиках в других местах

[Херовимчик]

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

Раздоры

Сочувствую. Если нет авторитета или authority то ничего не выйдет.

Ну вот авторитетом и будем мочить authority

[Komissar]

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

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

Вопрос никак уладить раздор. Вопрос о существующих практиках в других местах

мать, пора валить

[KVA]

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

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

Вопрос никак уладить раздор. Вопрос о существующих практиках в других местах

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

[nyekimov]

Был в разных ситуациях. Отсутсвие pull request review в команде больше одного человека считаю красным флагом. Сломал поломал хорошо ровно тогда, когда ты один и надо убирать только за собой и соответственно все карты на руках сделать это оперативно.

Чем больше команда и соотвественно проект тем, считаю, важней код ревью. Но должна быть какая то золотая середина. Например стилистика должна проверяться линтерами. CI тоже должен иметь это встроенно и репортовать о проблемах как только открывается pull request. У нас нет никаких Тулов, есть формат: jira ticket id period meaningful change description.
Я вообще ненавижу какие либо тулы кроме cli вечно эти тулы начинают тормозить в итоге.
С синдромом вахтёра тоже сталкивался. Когда человек будет придираться даже к грамматике в комментарии к тесту, который описывает что вообще тест должен делать.

Ну и согласен с комиссаром, гораздо важней наличие тестов. Чем стилистика.
Но считаю, что sme должны быть ревьюверами, иначе проходной двор, каждый дворник приходит и изобретает свой велосипед; вместо того, что лаконично дописать новую фичу в стиле модуля или вообще в коде может уже все иметься, надо просто строчку добавить, а народ начинает городить что то своё, так как не в состоянии понять, что надо сделать.

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

У нас кстати в большой команде самая большая проблема в том, что на все этапы пока так и не успели прикрутить ci и скажем релиз н работает с новыми изменениями, а вот н+1 они ломают. И потом этот н + 1 вечно встаёт из-за изменений снизу. Но ответственная команда(ы) занята релизом н, сроки горят и все дела. Им некогда проверять все резилы сверху, а ci только в процессе разработки ночных билдов всего и всея. Где то они уже есть и вечно падают, опять же никому пока до этого дела нет. Так и живем.