Comments 36
У меня вся команда ревьюит код- помимо остального это помогает им понимать откуда идут правила вида 'коммит недолжен быть большим' и они стараются так неделать.
Во-вторых при больших пулл реквестах если ты видишь косяк архитектуры и пишешь об этом, то часто все изменить становится больно-буквально пару больших мердж реквестов от джуна в которых видны проблемы дизайна помогают им понимать, что лучше разбивать пр на более мелкие гораздо лучше тысячи объяснений, что так делать нельзя.
В нашей компании тоже стараемся задействовать всех коллег в качестве ревьюеров. Но только эта мера не делает из людей хороших авторов, к сожалению. Я регулярно сталкиваюсь, например, с таким отношением к ревью, когда ревьюер говорит "ну мне тут очень сложный код отдали на ревью, поэтому я только нейминг по диагонали проверил". Культуру ревьюеров сформировать, пожалуй, ещё сложнее, чем культуру авторов. Слишком велик соблазн просто подождать два часа и нажать на кнопку "Approved", сделав вид, что ревью проведено. Если коллега ревью делает именно таким способом, то это никак не сделает его хорошим автором.
Я не утверждаю, что метод совсем не работает. Он ограниченно работает. Но более действенной нахожу обратную связь от ревьюеров. Когда ревьюер раз за разом терпеливо и спокойно объясняет автору, почему нужно разделить большой PR на несколько. Это на масштабе работает лучше, чем "раз ты дал мне PR на 5 тысяч строк, то на, почитай на 6".
Мы внутренние семинары ещё проводим. Как для авторов, так и для ревьюеров.
на масштабе работает лучше, чем "раз ты дал мне PR на 5 тысяч строк, то на, почитай на 6".
По-моему начинать давать плохие пулл-реквесты "потому что ты такой сам читал"-вообще порочная практика. Это будет замкнутый круг. Лиды должны слать максимально хорошие пулл реквесты, чтобы с них брали пример.
Но просто говорить что так плохо -тоже нелучший вариант-очень многие люди не делят пулл реквесты просто потому что не понимают зачем нужны маленькие пулл реквесты. Если же например 2 джуна начинают ревьюить друг друга они видят эти ошибки и потом стараются их недопускать.
Я регулярно сталкиваюсь, например, с таким отношением к ревью, когда ревьюер говорит "ну мне тут очень сложный код отдали на ревью, поэтому я только нейминг по диагонали проверил". Слишком велик соблазн просто подождать два часа и нажать на кнопку "Approved", сделав вид, что ревью проведено. Если коллега ревью делает именно таким способом, то это никак не сделает его хорошим автором.
Честно говоря не могу сказать что сталкиваюсь часто с таким отношением. Причем видел такое в ситуациях только когда код ревью проводится для "галочки"-на любые замечания у автора есть тысяча аругментов почему это надо было сделать именно так и по итогу споров просто ничего не правится.
У меня хорошо работал вариант когда я кого-либо назначал "главным" по код-ревью, и это были все люди на проекте по очерди-тогда этот человек начинает ревьюить проект вычитавая полностью. Так люди вовлекались в процесс.
По-моему начинать давать плохие пулл-реквесты "потому что ты такой сам читал"-вообще порочная практика. Это будет замкнутый круг. Лиды должны слать максимально хорошие пулл реквесты, чтобы с них брали пример.
+100
Но просто говорить что так плохо -тоже нелучший вариант-очень многие люди не делят пулл реквесты просто потому что не понимают зачем нужны маленькие пулл реквесты.
Если автора три раза попросили поделить большой PR, то в четвёртый раз он его уже поделит сам. Даже если он и не проникся, зачем это нужно. Но я тут не спорю с вашей точкой зрения. Гораздо лучше если автор проникся. И если он сам иногда также является ревьюером, то ему, однозначно, проникнуться будет проще.
Честно говоря не могу сказать что сталкиваюсь часто с таким отношением.
Редко кто в таком признаётся, конечно. Но по факту видно же, где ревьюер вчитался, а где шёл по диагонали.
Причем видел такое в ситуациях только когда код ревью проводится для "галочки"-на любые замечания у автора есть тысяча аругментов почему это надо было сделать именно так и по итогу споров просто ничего не правится.
Такое тоже видел. Если с автором невозможно нормально спорить, то нужно, чтобы с ним поговорил руководитель и объяснил, что это плохо. Ну а если не сработает, мы все знаем, что с таким автором нужно сделать :)
У меня хорошо работал вариант когда я кого-либо назначал "главным" по код-ревью, и это были все люди на проекте по очерди-тогда этот человек начинает ревьюить проект вычитавая полностью.
Интересный подход. Подумаю, можно ли у нас такое применить.
Также следует воздержаться от rebase и подобных действий.
Ну причесать историю перед отдачей на ревью без ребейза не выйдет (выйдет на самом деле, но это не самый лучший способ)
Бывает что ветка отстала от апстрима так, что мержкоммит стал размером с пуллреквест, такое тоже нельзя отдавать на ревью, как бы вылизан не был пуллреквест.
Любой нетривиальный конфликт должен быть разрешен разработчиком.
Причесал, ребейзнул на мастер, причесал ещё раз, отдал на ревью.
У самого на проекте очень часто на ревью приходят большие PR и соответственно, их ревьювят «мельком» (грешен, поскольку и сам так делаю).
Я применяю два способа, в зависимости от сложности ситуации. Если изменения практически независимы, то их можно отделить так:
- "Скинуть" изменения из истории через
git reset HEAD~. Если ваши изменения распределены по нескольким коммитам, то вместоHEAD~должен быть хэш коммита, предшествующего вашим изменениям. - С помощью
git add -iили пофайловымgit addили любыми аналогами, которые у вас под рукой, добавить изменения в коммит. Закоммитить. - Повторить пункт 2 для каждого отдельного изменения.
Второй способ позволяет отделять изменения, которые тесно связаны:
- Сохранить на всякий случай исходные коммиты в каком-нибудь отдельном бранче. Можно, в принципе, и просто где-то хэш коммита записать. Если что-то пойдёт не так, всегда можно будет начать сначала.
- "Скинуть" изменения так же, как в пункте 1 из предыдущего способа.
- Идём от обратного, удаляем изменения, которые не должны попасть в первую порцию изменений. Делаем это любым подходящим для вас способом. Часто это будет ваша любимая IDE.
- Получили первую порцию изменений без остального. Делаем
git add ...иgit commit. - Возвращаем исходные изменения в рабочую копию через
git checkout <хэш-из-п1> .. - Можем повторить шаги, начиная с 3, чтобы отделить ещё изменения.
Разумеется, это всё не какие-то волшебные способы, которые за вас могут отделить изменения за 10 секунд. Потребуется провести дополнительную работу. Обычно, это не занимает много времени, относительно времени, затраченного на написание исходных изменений. Но и удивляться не стоит, что на распутывание клубка из 3K+ строк и 10 связанных изменений нужно 2 часа. Это время окупается при ревью как временем ревьюера, так и качеством ревью.
Кажется вопрос не о том. Вопрос про декомпозицию задачи, когда задача большая и монолитная. Но тут никакие инструменты не помогут. Если таких задач много, то стоит пересмотреть куда более фундаментальные процессы в команде.
Я всем нашим разработчикам изначально настоятельно рекомендую декомпозировать большую задачу и одна из причин как раз та, что ревьювить всю задачу целиком нереально. Но, судя по всему, пока разработчик сам не попробует сделать ревью нескольких PR, включая большие, понимание проблемы к нему не придет. Поэтому пытаюсь приучить подключаться к ревью каждого члена команды.
в случае когда саму задачу сложно (невозможно) разбить на более мелкие
Я на эту фразу опирался. Гит же позволяет вам хоть по одной строчке коммитить. Переписать историю можно с помощью interactive rebase.
Вы описали разбиение на коммиты, но разбиение на коммиты не имеет никакого смысла.
Ревьювятся не коммиты, а результирующий дифф между веткой и мастером.
Как это, никакого смысла? Если всё в одном коммите, то как сделать разные Pull Request'ы для ревью?
Разбиение на коммиты является обязательным шагом перед выделением отдельных бранчей на ревью. Основная сложность связана именно с этим шагом. А когда отдельные изменения уже лежат по разным коммитам, нет никакой сложности из отдельных коммитов сделать отдельные бранчи.
Это в вашем процессе не имеет смысла, а в других — вполне имеет.
В Linux, например, ревьювятся отдельные коммиты из ветки. Вернее, набор коммитов.
Ядро операционной системы. Можете читать любой из их списков рассылки в поисках показательных примеров процесса разработки. И документацию на процесс.
Вот первый попавшийся набор патчей из LMKL, в котором есть дискуссия:
pragma once: treewide conversion
Набор из 11 патчей. Принимается или отклоняется набор в целом. Но каждый из патчей обсуждается отдельно, а не в виде мегадиффа. И при этом они не отправлены в виде 11 не связанных между собой патчей.
В смысле "не имеет смысла в одной ветке".
Если это вы меня процитировали, то я не вижу, чтобы я такое писал. Когда коммит разбивается на несколько, то изначально они в одной ветке находятся. Потом можно растащить по разным.
Разные пулл-реквесты должны быть из разных веток.
Вы абсолютно правы и я, вроде бы, нигде не писал обратного.
Имхо: никак, в таком случае должна быть декомпозиция на уровне гита.
Пример: необходимо реализовать большую фичу, чтобы ее реализовать необходимо выполнить следующие шаги:
- шаг 1 (отрефакторить)
- шаг 2 (добавить новую мелкую фичу)
- шаг 3 (реализовать большую фичу)
Ветка с большой фичей будет выглядеть следующим образом:
- merge "шаг 1" into "большая фича"
- merge "шаг 2" into "большая фича"
- merge "шаг 3" into "большая фича"
- commit 1
- ...
- commit n
Смысл в том, что крупные шаги можно отследить по merge commit'ам.
потратить порядка 10% от времени, затраченного на написание кода, на подготовку к ревью. Следует помнить, что ревьюеру на качественное ревью легко может потребоваться и 20%, и 50% от времени, затраченного автором на написание кода.
А сколько при таком раскладе будет уходить на исправление замечаний по ревью?
т.е. 10% подготовка, 20-50% само ревью. Если есть замечания, то сколько в среднем закладывается на них?
Мне кажется это ключевое в процессе. Т.е. убедить менеджмент что вы будете заниматься какой-то неизмеряемой деятельностью 30-60% процентов времени.
У нас, обычно, ревью проходят либо без запросов на изменения, либо с косметическими запросами. В редких случаях требуются значительные переработки. Но это те случаи, ради которых ревью и существует. Такие ревью фильтруют решения с существенными недостатками.
Плюс, ревью формируют определённую культуру, когда авторы, зная требования ревьюеров, заранее делают всё по этим требованиям. В условиях отсутствия ревью авторы могут и не знать всех нюансов, даже если они где-то описаны в документации. Ревью распространяет единую корпоративную культуру разработки в команде.
Вопрос "как убедить менеджмент" справедлив и для, например, написания автотестов. И автотесты, и ревью кода не добавляют стоимости продукту напрямую. Однако, я себе не представляю адекватного менеджмента, который был бы против автотестов и ревью. Видел в жизни обратное, когда автотесты и ревью внедряются менеджментом, а разработчики сопротивляются.
ну т.е. кто-нибудь занимается оптимизацией процесса(чтобы сделать быстрее\дешевле чем конкуренты) или просто есть внутренний бюджет и вы его расходуете?
Мы стараемся оптимизировать процессы, чтобы внесение изменений обходилось дешевле. Проект у нас "бесконечный", мы его будем развивать столько времени, сколько позволит бизнес. Поэтому, мы заинтересованы мыслить стратегически, заглядывая и на год, и на два, и на пять лет вперёд.
Не очень понял про "внутренний бюджет", т. к., очевидно, он у нас, в широком смысле, есть. Как и у всех.
Ну т.е. я понимаю что для программистов это наверное хорошо, но почему менеджмент это не присекает
дает не очень хорошие результаты(т.е. нет реджектов кода или их мало)
Ревью даёт прекрасные результаты! Малое количество реджектов это следствие наличия процесса ревью. Если бы ревью не было, то качество кода было бы ниже.
Это можно сравнить с ограничениями скорости на дорогах. Если нет камер и риски водителя ограничиваются редким взаимодействием с гаишником, которого относительно легко заметить, то много водителей будут ездить 120 по городу. Когда появляются автоматические камеры, то их эффективность оценивается не в руб./с, а в том, что водители начинают нарушать гораздо реже.
остатки отладочных конструкций, закомментированного кода, TODO-комментариев, бессмысленных изменений форматирования, не пригодившегося кода и прочего подобногоПо-моему, имеет смысл делать это перед каждым коммитом, не стоит откладывать до создания PR, где таких мусорных изменений может быть очень много. Ну и современные IDE имеют кучу встроенных pre-commit проверок, которые сильно упрощают эту задачу. Можно конечно последним коммитом делать эдакий «cleanup», но мне кажется проще коммитить уже чистый код.
Отличный пост, спасибо!
Спасибо!
Есть ли у вас какие-то формальные критерии, по которым определяете, нужно ли создавать Pull Request или можно коммитить и так?
Есть неформальный критерий, что тривиальные изменения можно мёржить без ревью. Определение "тривиальности" остаётся в качестве самостоятельного упражнения автору. Если что-то нехорошее всплывёт, что было занесено без ревью таким вот "тривиальным" изменением, с автором будет серьёзный разговор с внушением.
По-моему, имеет смысл делать это перед каждым коммитом
Если автор так делает, то автор — молодец! Не шучу. Я так не умею и всегда все хвосты подчищаю в самом конце. Я слышал также, что существуют люди, которые заранее всё правильно нарезают на коммиты. И без багов пишут!
Критерия "нужно ли создавать пулл-реквест" тут быть не может. Тут должно быть железное правило: никаких коммитов в мастер, все изменения — только через пулл-реквесты, всегда, без исключений.
Вы чересчур категоричны :)
Это зависит от требуемого баланса скорости разработки и стабильности продукта. Иногда бывает норм вообще не ревьюить, сразу в прод фигачить. А иногда — через три комиссии любое, сколь угодно тривиальное, изменение проводить.
Только если это ваш личный проект и вы там один участник.
Ну не только, конечно. Есть и другие кейсы. Например, стартап в фазе PoC. Например, проект с ограниченным сроком жизни, вроде программного обеспечения к какому-нибудь уникальному ивенту или промосайта. Уверен, этими примерами список не исчерпывается. Загоняя себя в такие категоричные рамки, вы отбираете у себя возможность гибко регулировать баланс скорость-качество.
Представим, что человек находит опечатку. И сразу чувствует, что уже не зря поработал. А что проку? На более важное у него уже нет энергии. Что печально, эту опечатку мог бы найти и автор кода с помощью утилиты анализа. Тем более что, хотя выявление опечаток требует больших усилий при ревью, эти ошибки всё равно великолепно остаются незамеченными.
Когда такие ошибки рассматриваются отдельно, они выглядят очень простыми. И даже удивительно, как они могут присутствовать в коде после того, как кто-то специально читал и проверял этот код. Но, по факту, у программистов не хватает внимания вчитываться в каждую букву, скобку, цифру.
Взгляните, например, на этот код:
p[0] = 0xfc | ((wc >> 30) & 0x01);
p[1] = 0x80 | ((wc >> 24) & 0x3f); // <
p[1] = 0x80 | ((wc >> 18) & 0x3f); // <
p[2] = 0x80 | ((wc >> 12) & 0x3f);Ошибка очевидна. Но очевидна она только сейчас, когда я привожу маленький фрагмент кода. В реальном же проекте такая ошибка отлично прячется и проходит обзор кода, оставаясь незамеченной. Никто не хочет вчитываться в каждый индекс массива. При этом речь идёт не о каком-то невнимательном студенте. Точно также ошибаются и профессиональные, опытные программисты. Например, эту ошибку я нашёл в таком известном проекте, как CMake.
Что интересно, такие ошибки не зависят от языка. Если подобная проблема кажется вам надуманной, то я вынужден буду вас разочаровать. Таких ошибок очень много и в своё время я написал целую статью на эту тему: Ноль, один, два, Фредди заберёт тебя.
Некоторый код программисты вообще почти не проверяют, так как это скучно. Примером могут служить однотипные функции для сравнения объектов. Проверять их неинтересно. Но то, что функция скучная, не означает, что там не может быть ошибки. Пример:
inline bool qCompare(QImage const &t1, QImage const &t2, ....)
{
....
if (t1.width() != t2.width() ||
t2.height() != t2.height()) // <
....
}Банально? Да, но это совершенно реальная ошибка из кода проекта Qt. И опять-таки на эту тему есть целая статья: Зло живёт в функциях сравнения.
Итак, при обзорах кода программисты часто не замечают ошибки. Что с этим делать? Использовать вспомогательные инструменты, такие как статические анализаторы кода. Эти программы, в отличие от человека, не устают и не ленятся :)
Спасибо за хорошую статью.
Спасибо!
Предлагаю ещё не забывать про статический анализ. <...>
Да, согласен. Всё, что можно доверить роботам, стоит отдать роботам. Дополню только, что помимо стандартного инструментария любой крупный проект должен обрасти своим, чтобы автоматически проверять принятые в проекте специфические правила и ограничения. На масштабе такое очень хорошо окупается.
Код ревью: как быть хорошим автором