Комментарии 26
Актуальная тема. По крайней мере для меня. Вчера сам перепроверял свой объёмный рефакторинговый пул-реквест, это было тяжело. Подумал, что надо было как-то поделить задачу на более мелкие, чтобы исправлений было не так много за один раз. Но для этого нужно заранее как-то уметь оценивать будущее число изменений в коде, что не всегда заранее возможно.
Тоже периодически отправлял на ревью пул-реквесты, которые оказывались слишком объёмными для одного ревью. В этом случае просто делил постфактум, создавая новые ветки с изменениями и накладывая PR друг на друга. В результате структура веток была примерно такой.
До:
- base
- big_pr -> base
После:
- small_pr_1 -> base
- small_pr_2 -> small_pr_1
- small_pr_3 -> small_pr_2
При подобной структуре любые существенные изменения в предыдущих PR ломают все последующие, не говоря уже о том, что за то время, пока последний PR дойдёт до проверки, в base могут упасть ещё чьи-то ломающие коммиты, из-за чего приходится постоянно копаться в конфликтах, но как будто это вынужденное зло и спасает ревьюера от вникания в тонны кода, а тебя - от проклятий с его стороны.
Перед ревью надо читать старый код. Чужой или свой не важно.
Если он читаем, то это хороший стандарт.
Но в 95%
Ну ты понял
«Ты выбрал плохой фреймворк, нужно было использовать X».
Кто вообще обсуждает фреймворк уже на код ревью? Мне кажется те, кто придумывают такие примеры вообще ни разу не проводили код ревью.
Правильно: «У тебя неплохое решение, но я знаю несколько альтернативных вариантов. Предлагаю обсудить».
Это неправильно. Либо предлагай что-то сразу, либо отвали.
Правильно: «Ты уверен, что переменные названы в соответствии с нашими стандартами оформления кода?»
Тоже неправильно. Да уверен. Всё?
(Кстати вот ещё один плюсик к тому что автор ни разу не проводил код ревью - обычно стандарты оформления кода не включают в себя правила называния переменных. Т.е. да, иногда стандарт может указывать, что например стринги в конце должны быть Str, или мы пользуемся snake_case, но формулировать это как "Ты уверен, что переменные названы в соответствии..." это странно. Здесь как будто автор подразумевает, что есть стандарт не называть перменные как 'a', 'b', 'c', хотя как бы обычно явного стандарта на такое нет)
Правильно: «У меня есть личное мнение насчёт этого фреймворка, давай обсудим?»
Неправильно. Опять же, ты хочешь обсудить, но не пишешь что именно.
А ещё, из-за того что это перевод, все эти добросердечные комментарии выглядят пассивной агрессией.
Я ещё в код ревью ставлю метку на комменты:
[C] comment - просто коммент, можешь прочитать и забыть
[R] request - это я считаю надо обязательно поменять
[Q] question - вопрос, можешь ответить и забыть
Если нет [R], то сразу и апрув
Половина замечаний это особенности перевода, а точнее другой культуры общения. Когда у нас скажут "передай сахарку", англичане скажут что-то типа "could you please give me a sugar". Первая фраза в буквалистском переводе звучит как команда строю солдат, а вторая как неуместные расшаркивания. При этом ни тем ни другим не является.
Для того, чтобы избежать на код-ревью вопросов про фреймворк, у нас есть дизайн-ревью, где все эти вопросы обсуждаются и утверждаются. Сохраняет много времени. Чем на более раннем этапе найден баг или высказано замечание, тем дешевле его исправлять.
Кмк есть стандарты нейминга - тут нет или не должно быть вопросов об уверенности
Работал в компании, где порой чересчур трепетно относились к качеству кода. У каждого сформировано своë мнение о качественном коде, отличающееся от других. Это приводило к спорам по несколько часов (чаще в асинхроне)
В какой-то момент техлид стал призывать всех не спорить о нейминге, потому что все примерно адекватно выбирали названия. И помечать незначительные комменты (обычно в конце коммента писали "не стоппер"). Помеченные комменты дуер мог просто закрыть и не спорить. После этого работать стало приятнее, ревью стало проходить заметно быстрее
Считаю, надо с уважением относиться к чужому времени и не душить по мелким поводам, и даже иногда бить себя по рукам, если хочется написать коммент об исправлении, которое почти не улучшит читаемость кода
У нас сделали статью по единому кодстайлу и для автоформатирования интегрировали, что было возможно, в правила для clang format. Теперь по одному нажатию хоткея код форматируется как надо.
Это очень правильно и хорошо.
Чем раньше это будет сделано тем больше выиграл в долгосрочной перспективе.
Нужно идти и дальше clang-tidy, editorconfig, pre-commit, как можно больше автоматизированной обработки кода.
В идеале добиться того, чтобы не было никакой надобности в комментариях по именованию , стиля и т.п.
А можно просто забить на стиль форматирования, не вставляя clang-format.
Если цель потратить побольше времени то конечно.
Обычно цель это уменьшить необходимое время на осмотр кода.
Сфокусироваться исключительно на логике, и как можно меньше на оформлении.
Для того, чтобы этого добиться у нас и есть вышеперечисленные решения.
А в современных языках сразу всё из коробки и даже нет возможности возражения.
Нет, я некоторое время работал в группе из "зрелых промышленных практиков с ипотекой и детьми", и никто форматированием не заморачивался: комментарии по форматированию просто не принимались во внимание и не писались. Если кому-то хотелось переформатировать, делался отдельный PR, без изменений логики, который просто утверждался после минутного просмотра и прохождения CI. Но такое желание возникало крайне редко.
Для меня код-ревью это в первую очередь общая ответственность за качество и корректность кода. Если в итоге в коде нашли баг, то не только автор, но и ревьювер ответственны за этот баг.
Кроме того, ревью - это возможность для команды узнать, как работают другие части проекта, а не только то, что они сами делают.
А если баг неочевидный? Все таки на реализацию могло уйти несколько дней, а ревью проводится гораздо быстрее, и не всегда можно в нем найти баг.
Вот если его и на тесте на нашли то да, ответственность тестировщика и разработчика, а ревьюера если только он халатно к ревью отнесся и не увидел очевидный баг
А каким инструментом Вы пользуетесь для чеков в пре-комиите?
Увидел комментарии про "лишние пробелы" в скриншоте из примера правильного ревью и немножко подгорел. И плохой, и хороший примеры там - это часть плохого явления. Имхо, ничего lint-related в код-ревью быть вообще не должно.
В моей первой компании, куда я устроился работать джуном, большая часть ревью у отдельных ревьюеров уходила на "тут лишняя запятая", "тут фигурная скобка не на той строке" и так далее. Ты джун, и так много всего нового и комплексного. Ты с этим разбираешься, но вместо полезного технического фидбэка получаешь ответы от линтера, просто в данном случае им выступает человек, а не софт. В этом ноль пользы и смысла для обеих сторон этого ревью. Часы, потраченные на выявление этих несоответствий и написание этих бессмысленных комментариев, лучше было заложить в настройку линтера и включение его в CI/CD и хуки гита.
К счастью, спустя год моей работы там линтер-таки был настроен. И это уменьшило фрустрацию от ревью в разы!
Все подпроцессы ревью, которые могут быть автоматизированы, должны быть автоматизированы и проверены до этапа самого ревью. Не прошёл линтер - нет ревью. Упали тесты - нет ревью. Тесты необоснованно лезут в сеть - нет ревью. Долгие запросы в тестах - нет ревью. Набор правил у всех свой, но какую-то общую и частую боль вроде линтера вынести за рамки ревью уж точно дёшево и несложно. Чем меньше ревьюер будет искать, а ревьюемый - править не те скобки, пробелы и запятые, тем больше они потратят времени на что-то более полезное и более значимое для приложения в целом.
Индусы в команде научились ловко саботировать ревью. т.е. на любую претензию - please guide me how to correct.
Далее начинается интерестно - или тебя нужно реально врубиться в задачу, чтобы сделать нормальный и детальный гайд(что отвлекает, занимает много времени и т.п.) или ты делаешь что-то высокоуровневое, что содержит ошибки реализации и потом все стрелки они пытаются свести к тебе. В целом способ рабочий чтобы все застопить или свести к поиску неверных отступов
Ты же понимаешь, что писать код за индусов вместо ревью это не выход. Достаточно небольшого аргументированного ответа почему ты его не принял, и не обязательно всего кода целиком просто до первого косяка. Если он будет месяц тебе один мердж задавать, либо индус исправиться либо руководство, и избавиться от индуса.
на любую претензию - please guide me how to correct
Хитро. Китайцы брали измором, меняли чуть ли не одну букву и делали новый PR. Когда число итераций подходило к 10 всех это уже задалбывало и апрувили)
please guide me how to correct
Не дешевле в какой-то момент уволить за профнепригодность и нанять более компетентного специалиста?
Тут же возникает вопрос а кто в этой ситуации с точки зрения менеджера более профнепрегоден. Тот кто задерживает процесс разработки создавая замечания, которые он же объяснить не может, или кто вежливо просит "гайдить". К тому же уволить(или даже начать это обсуждать) за то что твой код не нравится коллеге, это довольно странно, вы бы сами согласились работать в такой компании?
Если вопрос субъективного чувства красоты, то тогда я не понимаю причин возникновения конфликта интересов. Стиль и прочая мишура либо форсятся автоматически (clang-tidy и т.п.), либо один раз описывается в документе. Если в конкретном месте хочется точечно что-то подправить в рамках допустимого, то я не встречал такого чтобы мне писали "переименуй как-нибудь", всегда писали во что переименовать, или хотя бы в каком конкретно направлении подумать. Хот ьсколь-нибудь значимых задержек, соответственно, по этому поводу не возникало никогда.
Если же замечания объективные ("этот код нужно сделать потокобезопасным", "это ействие можно выполнять только если вон та фигня в нужном состоянии", и всё такое), то мой вопрос остаётся в силе.
А тут уже возникает вопрос, а сам менеджер то проф.пригоден? Если да, то он прекрасно разберётся кто саботирует процесс. Для этого у него есть куча людей: тим-лид, архитектор, старшие разработчики. Он может созвать совет и выслушать кто что видел и что думает. Не могу представить, что бы вменяемый тим-лид стал бы выгораживать индусов, которые по факту перекладывают работу на других.
Тут лишь вопрос, а чего же на самом деле хочет менеджер? Может его эти индусы вполне устраивают... Но тогда тут выход один - увольняться должна остальная команда, а они там пусть потом сами. Через полгодика будут обзванивать с предложением вернуться и тут главное не продешевить ))
Что касается самой статьи, то на мой взгляд верно только частично. Если код-ревью обязательный процесс, то оставлять там комментарии в стиле "мне кажется, что здесь лучше по другому" можно только в очень редких случаях, потому что это не будет исправлено. Особенно если ревью делает тим-лид или сеньор, а автор пула - джун или мидл. Там должно что-то вроде "здесь не верно, потому что... " Если с той стороны возникнут вопросы, их можно обсудить, но комментарии должны носить директивный характер иначе это трата времени. И само собой есть какой то минимальный уровень до которого можно опускаться в объяснениях. Если человека наняли на мидла, а он задаёт вопросы "how to correct" на элементарный комментарий, значит он не соответствует уровню и либо он подтягивается, либо ставится вопрос о его пригодности перед лицом принимающим решения.
В целом хорошо написано, но мне кажется зацепили много вещей не касающихся ревью, а относящихся к процессу тестирования. Ну и ужасные примеры вроде пробелов в конце строки, при сравнении кода обычно системы контроля версий их подсвечивают как изменения не имеющие значимости. Подобные вещи это имитация бурной деятельности а не ревью. При корректной постановке задач, обычно куски не большие а форматирование вообще не рекомендуется трогать, т.к. это просто усложняет читаемость изменений ревьюером. Тут либо общий формате при сборке, либо вообще не трогать и запрещать форматировать уже написанный код.
Я бы сказал что если применить сразу все практики из статьи, ревью такого проекта станет полностью не эффективным и будет просто бюрократией, а не полезным инструментом.
Самые важные 2 пункта это нейминг и единый ревьюер, в пределах микросервиса.
Делаем код-ревью правильно