Code Review

nyekimov
Уже с Приветом
Posts: 2749
Joined: 11 Jul 2015 19:01
Location: Chicago

Re: Code Review

Post by nyekimov »

KVA wrote: 28 Jul 2021 02:38
Херовимчик wrote: 28 Jul 2021 02:21
alex_127 wrote: 28 Jul 2021 02:10
Херовимчик wrote: 28 Jul 2021 01:56 прекратить... что именно? Поломки билдов, баги в софте или раздоры между группами?
для этого совета совсем недостаточно данных.
как можно давать совет по ср если проблемы похоже глубже?
ответ верхнего уровня - нужен правильный хороший деятельный менеджер. поставить туда.
Вопрос никак уладить раздор. Вопрос о существующих практиках в других местах
Ну практики как бы очевидны. :pain1: От полной анархии и вольницы, до увольнения за сломанный билд (наверное где-то и такое есть). По статистике скорее всего практики где-то посередине. IMHO знание того что делается в другом месте никак не поможет решить вашу конкретную проблему. Ваша проблема не в технической области, а в области менеджмента и authority. Короче, кулаком надо стукнуть, сказать как надо (вы и сами сможете сообразить как надо) и наказывать за как не надо (а вот тут у вас authority-то и нет). Так что проблема не решаемая на вашем уровне.
Хороший коммент; поддерживаю.

Если sme выкатить идеальный pull request, то в чем проблема?

Чаще я вижу проблемы, когда человек не удосуживается исправляться, поработать над читательбностью. Оперативно не реагирует на вопросы или просьбы исправить код. Вместо этого бежит жаловаться, что его прессуют. И тут уже очень сильно зависит от менеджмента, кто то соображает поставить «спеца» на место, а некоторые менеджера даже начинают ругать ревьюверов. И вот для этого кстати не мешал бы свод правил, чтобы все знали критерии хорошего pull request -а. Свод правил будет позволять поставить и «вахтёра» на место.
User avatar
Херовимчик
Уже с Приветом
Posts: 5283
Joined: 27 Sep 2008 21:48
Location: Moscow-Seattle-SFBA

Re: Code Review

Post by Херовимчик »

Komissar wrote: 28 Jul 2021 02:36
Херовимчик wrote: 28 Jul 2021 02:21
alex_127 wrote: 28 Jul 2021 02:10
Херовимчик wrote: 28 Jul 2021 01:56 прекратить... что именно? Поломки билдов, баги в софте или раздоры между группами?
для этого совета совсем недостаточно данных.
как можно давать совет по ср если проблемы похоже глубже?
ответ верхнего уровня - нужен правильный хороший деятельный менеджер. поставить туда.
Вопрос никак уладить раздор. Вопрос о существующих практиках в других местах
мать, пора валить
Да ладно, и не таких мочили :mrgreen:
User avatar
Херовимчик
Уже с Приветом
Posts: 5283
Joined: 27 Sep 2008 21:48
Location: Moscow-Seattle-SFBA

Re: Code Review

Post by Херовимчик »

KVA wrote: 28 Jul 2021 02:38
Херовимчик wrote: 28 Jul 2021 02:21
alex_127 wrote: 28 Jul 2021 02:10
Херовимчик wrote: 28 Jul 2021 01:56 прекратить... что именно? Поломки билдов, баги в софте или раздоры между группами?
для этого совета совсем недостаточно данных.
как можно давать совет по ср если проблемы похоже глубже?
ответ верхнего уровня - нужен правильный хороший деятельный менеджер. поставить туда.
Вопрос никак уладить раздор. Вопрос о существующих практиках в других местах
Ну практики как бы очевидны. :pain1: От полной анархии и вольницы, до увольнения за сломанный билд (наверное где-то и такое есть). По статистике скорее всего практики где-то посередине. IMHO знание того что делается в другом месте никак не поможет решить вашу конкретную проблему. Ваша проблема не в технической области, а в области менеджмента и authority. Короче, кулаком надо стукнуть, сказать как надо (вы и сами сможете сообразить как надо) и наказывать за как не надо (а вот тут у вас authority-то и нет). Так что проблема не решаемая на вашем уровне.
У нас не принято пускать в ход authority (обе оппозиции имеют одинаковый вес тут). Верх инженерной мысли как раз найти совместное решение. И тут чужой опыт хороший data point
User avatar
Херовимчик
Уже с Приветом
Posts: 5283
Joined: 27 Sep 2008 21:48
Location: Moscow-Seattle-SFBA

Re: Code Review

Post by Херовимчик »

nyekimov wrote: 28 Jul 2021 02:49 Был в разных ситуациях.
Вот, вы меня хорошо поняли! Одно дело пара инвалидов и 3 PR в месяц, другое дело пол сотни народу, с десятками PR в день и частыми релизами
User avatar
KVA
Уже с Приветом
Posts: 5382
Joined: 03 Feb 1999 10:01
Location: NJ, USA

Re: Code Review

Post by KVA »

Херовимчик wrote: 28 Jul 2021 03:16 У нас не принято пускать в ход authority (обе оппозиции имеют одинаковый вес тут). Верх инженерной мысли как раз найти совместное решение. И тут чужой опыт хороший data point
Тогда усядемся получше и бум наблюдать шоу. :food: :D
mister-X
Уже с Приветом
Posts: 415
Joined: 31 May 2007 21:39
Location: Atlanta

Re: Code Review

Post by mister-X »

:
KVA wrote: 28 Jul 2021 03:38
Херовимчик wrote: 28 Jul 2021 03:16 У нас не принято пускать в ход authority (обе оппозиции имеют одинаковый вес тут). Верх инженерной мысли как раз найти совместное решение. И тут чужой опыт хороший data point
Тогда усядемся получше и бум наблюдать шоу. :food: :D
+1
Компетишен внутри теамс, когда амбиции девелоперов заставляют работать их больше и быстрее. Отличная стратегия. Пилите Шура, пилите :D
User avatar
Херовимчик
Уже с Приветом
Posts: 5283
Joined: 27 Sep 2008 21:48
Location: Moscow-Seattle-SFBA

Re: Code Review

Post by Херовимчик »

mister-X wrote: 28 Jul 2021 03:49 :
KVA wrote: 28 Jul 2021 03:38
Херовимчик wrote: 28 Jul 2021 03:16 У нас не принято пускать в ход authority (обе оппозиции имеют одинаковый вес тут). Верх инженерной мысли как раз найти совместное решение. И тут чужой опыт хороший data point
Тогда усядемся получше и бум наблюдать шоу. :food: :D
+1
Компетишен внутри теамс, когда амбиции девелоперов заставляют работать их больше и быстрее. Отличная стратегия. Пилите Шура, пилите :D
Ну вот оппозиция также как и вы мыслит похоже :oops: что если согнать по-больше крутых перцев, то все можно решить в пользу «сильнейших». При этом их не смущает что вопрос с годами встал только острее :pain1:
mister-X
Уже с Приветом
Posts: 415
Joined: 31 May 2007 21:39
Location: Atlanta

Re: Code Review

Post by mister-X »

Из собственного опыта я пришёл к выводу что 'инициативным' не надо мешать. Им надо добавлять работы чтобы не было сил отвлекаться на высокомерие по отношению к другим. Умный менеджер всегда таких использует. Не ввязывайтесь в чужую драку. Начальству это и нужно - компетишен чтобы не надо было мотивировать деньгами.
User avatar
Херовимчик
Уже с Приветом
Posts: 5283
Joined: 27 Sep 2008 21:48
Location: Moscow-Seattle-SFBA

Re: Code Review

Post by Херовимчик »

mister-X wrote: 28 Jul 2021 04:27 Из собственного опыта я пришёл к выводу что 'инициативным' не надо мешать. Им надо добавлять работы чтобы не было сил отвлекаться на высокомерие по отношению к другим. Умный менеджер всегда таких использует. Не ввязывайтесь в чужую драку. Начальству это и нужно - компетишен чтобы не надо было мотивировать деньгами.
Мне за обязанность Бабы-Яги/Миротворца как раз хорошо доплачивают :mrgreen: а нормальному начальству как раз нужно перенаправить излишки энергии в продуктивное русло, а не устраивать rat race на ровном месте :fr:
alex_127
Уже с Приветом
Posts: 7723
Joined: 29 Mar 2000 10:01
Location: Kirkland,WA

Re: Code Review

Post by alex_127 »

Херовимчик wrote: 28 Jul 2021 03:16 У нас не принято пускать в ход authority (обе оппозиции имеют одинаковый вес тут). Верх инженерной мысли как раз найти совместное решение. И тут чужой опыт хороший data point
мое мнение (может конечно неправильное) - как раз нужен authority. поскольку проблемы детского сада. вот если бы действительно непонятно было что и как делать то cr были бы одним из ваших наименьших проблем...
User avatar
Херовимчик
Уже с Приветом
Posts: 5283
Joined: 27 Sep 2008 21:48
Location: Moscow-Seattle-SFBA

Re: Code Review

Post by Херовимчик »

alex_127 wrote: 28 Jul 2021 06:24
Херовимчик wrote: 28 Jul 2021 03:16 У нас не принято пускать в ход authority (обе оппозиции имеют одинаковый вес тут). Верх инженерной мысли как раз найти совместное решение. И тут чужой опыт хороший data point
мое мнение (может конечно неправильное) - как раз нужен authority. поскольку проблемы детского сада. вот если бы действительно непонятно было что и как делать то cr были бы одним из ваших наименьших проблем...
Однозначно ответа как делать правильно - нет. У всех подходов есть свои плюсы и минусы (наш продукт существует лет 8 уже, и много что было перепробовано, много шишек набито). Каждая сторона по-своему права.

Вообще удивительно что большинство воспринимает разногласия в процессах/мнениях между командами как конфликт, который требует вмешательства authority. Обычный рабочий момент :pain1:
alex_127
Уже с Приветом
Posts: 7723
Joined: 29 Mar 2000 10:01
Location: Kirkland,WA

Re: Code Review

Post by alex_127 »

Херовимчик wrote: 28 Jul 2021 15:27
alex_127 wrote: 28 Jul 2021 06:24
Херовимчик wrote: 28 Jul 2021 03:16 У нас не принято пускать в ход authority (обе оппозиции имеют одинаковый вес тут). Верх инженерной мысли как раз найти совместное решение. И тут чужой опыт хороший data point
мое мнение (может конечно неправильное) - как раз нужен authority. поскольку проблемы детского сада. вот если бы действительно непонятно было что и как делать то cr были бы одним из ваших наименьших проблем...
Однозначно ответа как делать правильно - нет. У всех подходов есть свои плюсы и минусы (наш продукт существует лет 8 уже, и много что было перепробовано, много шишек набито). Каждая сторона по-своему права.

Вообще удивительно что большинство воспринимает разногласия в процессах/мнениях между командами как конфликт, который требует вмешательства authority. Обычный рабочий момент :pain1:
Ну так как раз чтобы не было переливания и долгих бесполезных споров. "Мы посовещались и я решил"
User avatar
KVA
Уже с Приветом
Posts: 5382
Joined: 03 Feb 1999 10:01
Location: NJ, USA

Re: Code Review

Post by KVA »

Херовимчик wrote: 28 Jul 2021 15:27 Однозначно ответа как делать правильно - нет. У всех подходов есть свои плюсы и минусы (наш продукт существует лет 8 уже, и много что было перепробовано, много шишек набито). Каждая сторона по-своему права.

Вообще удивительно что большинство воспринимает разногласия в процессах/мнениях между командами как конфликт, который требует вмешательства authority. Обычный рабочий момент :pain1:
Если за 8 лет не нашли согласия в процессах/мнениях, то и сейчас не найдете если будете nice. Если конфликт нужно решить сейчас, то это IMHO возможно только через authority (делай как я сказал, а не то ... :evil: )
User avatar
Vladimir Kr.
Уже с Приветом
Posts: 571
Joined: 24 Mar 2004 07:31
Location: Krasnoyrsk -> -> Chicago

Re: Code Review

Post by Vladimir Kr. »

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

- Если людей не хватает, то решается делегированием:
конкретно в вашем примере: (#2) должны проверять пре-апрувы доверенных TL&SME, a не ревьювить весь PR сами
- Вопросы архитектуры не должны всплывать на PR кода, архитектура решается до создания бранча
- самопиленные тулзы для линка систем? зачем, есть jira с прозрачной интеграцией процесса (тикет->бранч->PR->build->тикет), или аздо девопс gates, например https://docs.microsoft.com/en-us/azure/ ... -approvals (бранч->PR->deploy->test->merge)
- code style, code coverave, TDD/BDD, все это может SME/TL проверить - есть-ли покрытие фичи integration/functional test, или нет, соответственно апрувнуть, % покрытия и прочее; или автоматизировать - см. gates выше

вот если PR собираются, чтобы "научить и доказать", тогда надо звать authority

формализуйте правила PR не для бюрократии, а для удобства работы
моя родина СССР!
User avatar
Херовимчик
Уже с Приветом
Posts: 5283
Joined: 27 Sep 2008 21:48
Location: Moscow-Seattle-SFBA

Re: Code Review

Post by Херовимчик »

Vladimir Kr. wrote: 28 Jul 2021 16:13 все смешалось... PR с вопросами по архитектуре? задерживатся? и собираются вместе? из-за того, что не линкованы с другими системами?

- Если людей не хватает, то решается делегированием:
конкретно в вашем примере: (#2) должны проверять пре-апрувы доверенных TL&SME, a не ревьювить весь PR сами
- Вопросы архитектуры не должны всплывать на PR кода, архитектура решается до создания бранча
- самопиленные тулзы для линка систем? зачем, есть jira с прозрачной интеграцией процесса (тикет->бранч->PR->build->тикет), или аздо девопс gates, например https://docs.microsoft.com/en-us/azure/ ... -approvals (бранч->PR->deploy->test->merge)
- code style, code coverave, TDD/BDD, все это может SME/TL проверить - есть-ли покрытие фичи integration/functional test, или нет, соответственно апрувнуть, % покрытия и прочее; или автоматизировать - см. gates выше

вот если PR собираются, чтобы "научить и доказать", тогда надо звать authority

формализуйте правила PR не для бюрократии, а для удобства работы
Так у нас и так делегирование - одни смотрят только со стороны архитектуры, другие только с точки зрения домена, остальные по желанию и усмотрению
User avatar
Херовимчик
Уже с Приветом
Posts: 5283
Joined: 27 Sep 2008 21:48
Location: Moscow-Seattle-SFBA

Re: Code Review

Post by Херовимчик »

Оказалось как всегда, решали неправильную проблему. Доморощенный тул пытаются пропихнуть, т.к. он родной и понятный, и не надо «позориться публично». А всего-то нужно в письменной форме огласить все правила и ожидания, чтобы всем было понятно :?
User avatar
valchkou
Уже с Приветом
Posts: 4185
Joined: 27 Apr 2011 03:43
Location: Сергели ->Chicago

Re: Code Review

Post by valchkou »

Херовимчик wrote: 27 Jul 2021 16:01 А давайте поговорим о безопасных (от дураков) и эффективных практиках.

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

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

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

#1 качество такое, что народ набил руку на формат и все равно через UI делает (не, есть такие что неделями будут сидеть и ждать пока починят, остановив всю работу).
А как у вас?
1. у нас сонарклауд
2 и 3 это один и тот же человеки.
code ownership это пожалуй один из основных вызовов современной кодинг индустрии.
Я лично не понимаю как можно делать качественный ревью не будучи глубоко в теме и не являясь активным контрибьютором проекта.
Мы делаем все возможное чтобы у проекта таких было как минимум 2-е.
ревью довольно интенсивный и трудоемкий процесс. Порой проще переписать часть кода чтобы объяснить инженеру как нужно делать, чем оставлять бесконечные комментарии в UI.

Отсюда вопрос - а сколько активных проектов в состоянии переварить ревьювер чтобы и ревью качественно делать и свои задачи выполнять?
если мы говорим о больших изменениях где нужны дизайн паттерны и 100 грамм, то по моему опыту это до 3х активных проектов типа 20тыс строк и не более 2х ревью в день.
Поэтому 1 человек не сможет ревьювить команду даже из 10ти чел менеджить сможет а код ревиювить нет.
2 ревьювера на 5 кодеров по мне оптимально.
User avatar
Херовимчик
Уже с Приветом
Posts: 5283
Joined: 27 Sep 2008 21:48
Location: Moscow-Seattle-SFBA

Re: Code Review

Post by Херовимчик »

Спустя месяц…

Выкатали чёткие и краткие требования по стилю, в письменном и доступном виде.
Пересмотрели ownership, убрали кучу ненужных зависимостей.
Могут же, если пнуть в нужном направлении и предать собственное эго!
nickb
Уже с Приветом
Posts: 3207
Joined: 08 Aug 1999 09:01
Location: Tampa, FL

Re: Code Review

Post by nickb »

А у нас для feature запускается свой build. Со всеми проверками, sonarqube и т.п. Без такого build-а даже нельзя сделать MR.
А code review делают тим лиды этих девелоперов :)
Ignorance is bliss
User avatar
Херовимчик
Уже с Приветом
Posts: 5283
Joined: 27 Sep 2008 21:48
Location: Moscow-Seattle-SFBA

Re: Code Review

Post by Херовимчик »

nickb wrote: 27 Aug 2021 01:52 А у нас для feature запускается свой build. Со всеми проверками, sonarqube и т.п. Без такого build-а даже нельзя сделать MR.
А code review делают тим лиды этих девелоперов :)
Билды по всем существующим таргетам это святое!

Return to “Работа и Карьера в IT”