Часть вторая. Как проходить code review по версии Google

    Возможно вы читали первую часть статьи про код ревью со стороны ревьювера (кстати, мы уже успели ее обсудить в последнем выпуске подкаста "Цинковый прод").

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

    Как обычно, будем говорить MR (Merge Request) вместо CL, потому что термин CL мало кто понимает.


    Оригинал инструкции для авторов MR по версии Google можно посмотреть здесь, а я дам краткую выжимку.


    Итак, поехали


    Описание MR


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


    В первой строчке (в заголовке) должно быть одной фразой описано, что было сделано в MR. Причем по традиции применяется императивный (повелительный) стиль, т.е. Delete blablabla, а не Deleting blablabla


    Само описание должно быть информативным, содержать краткое описание решаемой проблемы, ссылки на необходимые документы (если необходимо), задачи таск трекера и другой контекст. Даже в маленьком MR что-то такое должно быть.


    Описание типа "Fix bug", понятное дело, считается неадекватным.


    MR должен быть как можно меньше


    • Маленький MR можно быстро проверить
    • Проверка будет более осмысленной
    • Меньше вероятность упустить баг
    • Не так обидно, если весь MR будет отклонен. Ведь очень плохо, когда сделана большая работа, а потом выясняется, что все это вообще было не нужно
    • Проще вливать изменения, меньше конфликтов
    • Легче добиться хорошего качества кода
    • Чем больше изменений за раз, тем сложнее откатывать код при такой необходимости

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


    Конечно, не может быть жесткого правила, какой MR будет считаться большим, какой маленьким. 100 строк кода — это уже большой или еще нет? Кто знает.


    • MR должен делать что-то одно. Обычно это не вся фича, а ее часть
    • Выделяйте рефакторинг в отдельный MR

    MR должен быть маленьким, но самодостаточным


    • Всё, что необходимо ревьюверу для понимания MR, должно быть в этом MR
    • После вливания кода система должна функционировать нормально.
    • Если добавляется метод API, в этом же MR должны быть продемонстрированы и способы его использования. Другими словами, MR не должен быть настолько маленьким, чтобы трудо было понять как он будет использоваться, к каким последствиям приведет.
    • Покрывайте код тестами, причем тесты должны быть в этом же MR

    Не принимайте близко к сердцу


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


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


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


    Из моего опыта хочу добавить, что зачастую человек пишущий невежливый комментарий, часто не отдает отчет, что это может смотреться как-то агрессивно. Текст скрывает половину эмоций; например, сказанное в шутливо-дружественном тоне в текстовом виде может казаться жестким наездом. Поэтому полностью согласен с гуглом — в случае недопонимания общаться только лично!


    Объясняйте кодом


    Если вас просят объяснить какой-то момент в коде, подумайте, нельзя ли поправить код так, чтобы он был понятен без объяснений. Потому что если один не понял, то и другие могут не понять.


    Реакция на комментарии ревьювера


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


    Однако если ревьювер все же не прав, смело пишите об этом, снабдив ответ аргументами и фактами.


    Выводы


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


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


    В следующем выпуске "Цинкового прода" мы обязательно обсудим эту статью (и многое другое), поэтому не забудьте подписаться на наш подкаст, иначе пропустите самое интересное!

    Support the author
    Share post
    AdBlock has stolen the banner, but banners are not teeth — they will be back

    More
    Ads

    Comments 35

    • UFO just landed and posted this here
        0
        MR должен делать что-то одно. Обычно это не вся фича, а ее часть

        Увы, это не всегда возможно. К примеру, MR, над которым я работал 2 месяца, состоит из 409 коммитов, 178 дискуссий о code review, затрагивает 18 файлов исходников (сколько строк — не считал, но куда больше 100). И разбить на части это ну никак, тут речь шла о тотальной переделке функционала, затрагивающего много чего. Да, какие-то мелкие пред-MR были, само собой. Уровня «создать новые таблицы в БД», «запустить скрипт миграции» и т.д. Но любое, даже самое мелкое изменение функционала без учёта зависимостей, ломало бы действующую систему.

        Да, это баг изначальной архитектуры (особенно с учётом того, что большая часть меняемого кода лежала в неймспейсе «Legacy» 10-летней давности), но уж с чем приходится работать, с тем и работаем :)

        Если вас просят объяснить какой-то момент в коде, подумайте, нельзя ли поправить код так, чтобы он был понятен без объяснений. Потому что если один не понял, то и другие могут не понять.

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

        Если добавляется метод API, в этом же MR должны быть продемонстрированы и способы его использования.

        У нас принято не так. Code review — это только про код. Способы использования, тесткейсы и прочее — в трекере, который, понятное дело, связывает задачу с MR, и делегирует дальнейшие полномочия (после аппрува MR и выкладки результата на тестовую виртуалку) в тестлаб. Не могу сказать как правильней, поскольку сталкивался в основном именно с такой моделью.
          +1

          И сколько дней ваш PR смотрели?

            –3
            Попробуй научиться читать текст русскими буковками на русском языке:

            работал 2 месяца, состоит из 409 коммитов, 178 дискуссий о code review


            А что такое PR?
              +2
              PR = Pull Request, другое название для MR.
              Интересно, как 409 коммитов сделать на 18 файлах? За 2 месяца — то есть 10 коммитов в день в среднем.
              И всё таки присоединюсь к вопросу — сколько дней ревьювер смотрел это всё?
              Русскими буквами написано сколько времени Вы это писали, а не сколько времени проходило ревью.
                +1
                У меня есть ощущение, что тут как раз и «выстрелила» сделанное автором статьи «упрощение» и переименование CL в MR.

                Потому что ну не верю я в то, что на эти 409 коммитов кто-то по отдельности смотрел.

                А в Гугле CL — это как раз одно атомарное изменение. И, соответственно, один commit. Оно может не делать ничего полезного (например может просто переименовывать поля в базе данных, ничего больше не делая), но оно атомарно и решает какую одну задачу. И оно либо механическое (переименовали childs в children и изменили все места где эти childs использовались), либо, наоборот, осмысленное и небольшое.

                При этом мы вполне можете реализовать любой рефакторинг как набор таких вот изменений и когда вы их рассматриваете, то вы можете на соседние изменения ссылаться… в сущности ничего нового: именно так всегда и вносились изменения в ядро Linux.
                  +2
                  Давняя шутка про ревью (лень искать источник):
                  10 lines of code = 10 issues
                  500 lines of code = looks fine
                    0
                    Потому что ну не верю я в то, что на эти 409 коммитов кто-то по отдельности смотрел.

                    Да, конечно, не на каждый же коммит :) И тут ещё одна особенность: push запускает CI, после его успешного завершения можно переводить таску в «code review». Независимо от создания MR. А на каждый пуш может быть и один коммит, и пара десятков, и сотня. Если есть аппрув по всем вопросам от команды — идём дальше. Иногда в процессе создаются тестовые машины и привлекаются тестеры.

                    А в Гугле CL — это как раз одно атомарное изменение. И, соответственно, один commit

                    Да, бывает и так: какое-то мелкое косметическое изменение сделал.
                    Но когда сталкиваешься с кучей legacy, которая просто не может жить в нынешних условиях, буквально: тут чуть ковырнул — посыпалось здесь, однострочными изменениями никак :(
                      0
                      Но когда сталкиваешься с кучей legacy, которая просто не может жить в нынешних условиях, буквально: тут чуть ковырнул — посыпалось здесь, однострочными изменениями никак :(
                      Ну вот как-то у разработчиков Android и Chrome это же получается. Вы думаете там мало легаси?

                      И речь не идёт об однострочных изменениях. Речь идёт об атомарных коммитах. Некоторые коммиты в Chrome могут и несколько тысяч строк включать — но только если это механические изменения…
                        0
                        Речь идёт об атомарных коммитах

                        Так, стоп. С нашей точки зрения, коммит — это внутреннее дело. Одну строчку, букву поправил, сохранил. У себя. Это коммит.
                          +1
                          Одну строчку, букву поправил, сохранил. У себя.
                          Кого вообще волнует что у вас там «у себя» в приватном репозитории хранится? Речь идёт о публичных коммитах. Вот примерно таких (не надо искать конкретно в этих глубокого смысла, я просто взял первый попавшийся Merge Request в LKML… как известно именно для разработки ядра и был создан Git, потому я его использую в качестве модели).

                          Обратите внимание, что каждый коммит имеет смысл, что в первом из них — указано, сколько их всего (хотя это частная фишка LKML, в Android и Chrome этого не требуют) и так далее.

                          То есть публичный коммит — это специально подготовленная вещь. С описанием и прочими атрибутами.

                          А что вы там делаете у себя в приватном репозитории — это ваше личное дело, в общем-то. Это никого не волнует. Собственно именно потому что это ваше внутреннее дело Git (в отличие от Mercurial) и не хранит информацию о ветках и прочем. Это — ваша личная история. Считается что перед показом её кому-то — вы её причешете. У Git'а есть для этого инструменты…

                  0
                  Проблемы с внимательность по ходу. Сколько ревьювили твоё творение, а не сколько оплачиваемого времени ты на него потратил, так понятнее?
                    0
                    Да уж, автор так составил предложение, что мне показалось, что это он не вносил изменения два месяца и сделал мерж-реквест, а ему дали мерж-реквест и он два месяца его его облагораживал.
                +1
                К примеру, MR, над которым я работал 2 месяца, состоит из 409 коммитов, 178 дискуссий о code review, затрагивает 18 файлов исходников (сколько строк — не считал, но куда больше 100)
                Если это что-то одноразовое то можно наверное понять. Но если это ежедневная реальность, то вы делаете что-то не то. Безотносительно к код ревью вообще.
                0
                del, мимо.
                  0
                  Причем по традиции применяется императивный (повелительный) стиль, т.е. Delete blablabla, а не Deleting blablabla
                  По традиции? А есть чуть более логическое обоснование? Потому что я не вижу здесь никакой разницы.
                  Не так обидно, если весь MR будет отклонен. Ведь очень плохо, когда сделана большая работа, а потом выясняется, что все это вообще было не нужно
                  Такие вещи должны вылезать гораздо раньше чем код ревью. Если кто-то работает втайне над какой-то никому не нужной хренью, то это факап менеджеров, тимлидов и т.д. К этапу код ревью такие вещи вообще не должны доходить.
                    +1
                    Да, в документе было именно «по традиции».
                      +1
                      Правильно, это стандартный выбор между А и Б, когда нет объективного преимущества одного над другим, но смесь из обоих это плохо. Тогда можно вначале хоть монетку бросить, а потом уже «придерживаться традиции».
                      0
                      По традиции? А есть чуть более логическое обоснование? Потому что я не вижу здесь никакой разницы.


                      Определенное обоснование есть (см. 5. Use the imperative mood in the subject line).

                      В целом большинство соглашений в программировании написаны конечно не кровью, как правила техники безопасности, — но некоторым образом выстраданы.
                        0
                        Ну не знаю что тут выстрадано:
                        One reason for this is that Git itself uses the imperative whenever it creates a commit on your behalf.
                          0
                          лично я считаю, что такие правила вредны. Мало ли какие традиции, и что там создает git при мерже. Разрабу нужно помнить еще одно бесполезное правило, которое не решает никаких проблем
                            0
                            “… какая разница – писать ли «моршрут» или «маршрут», «велосипед» или «виласипед»? От этого ведь велосипед мотоциклом не становится. Важно только, чтобы всё было понятно. А какая там буква в середине стоит — «а» или «о», — это, помоему, совершенно безразлично. И зачем только люди сами себе жизнь портят? Когда-нибудь они одумаются и отменят сразу все орфографические правила.”
                            (с) А.Алексин

                            Правила вроде орфографических, coding conventions, commit messages conventions и т.д. вроде бы практического смысла не несут, но позволяют читать код/текст чуть-чуть быстрее. Итоговая экономия времени может быть ощутимой.
                            Собственно смысл всех этих правил — сделать жизнь тех кто будет разбираться в коде/ревьювить реквест чуть проще и приятней. Чтобы могли разобраться быстро и без крови из глаз.
                            При работе в одиночку всё это не нужно, конечно.
                      0
                      Вариант, когда я лично не согласен с некоторыми аспектами:
                      Работаю над проектом. Прилетает задача: сделать окно с конфигурированием чего-то. Но вот архитектора нет, аналитиков нет, прочих руководителей нет — есть только я, абстрактная задача от менеджера (который не является программистом) и задача.
                      Я создаю новую ветку и начинается творческий процесс: прикидываю UI, пишу логику, создаю модели, проверяю/пробую… и вот эта череда действий много повторяется и при этом часто что-то стирается, переделывается и т.д. Т.е. идёт творческий процесс разработки с нуля. Ну вот сложно тут создавать какие-то умные пояснения к коммитам! Лично я в этом случае каждый день делаю коммит «daily commit».
                      А вот когда эта абстрактно-творческая фича запилена — вот тогда внесение правок, исправление багов и прочее уже можно делать полностью соответствуя рекомендациям в статье
                        0
                        Ну вот сложно тут создавать какие-то умные пояснения к коммитам! Лично я в этом случае каждый день делаю коммит «daily commit».
                        А какая вообще разница что вы там пишите у себя в репозитории? Хоть на смеси матерного и суахили выражайтесь. Главное — что ревьюер их не видит и в публичном репозитории их нету.

                        А вот когда эта абстрактно-творческая фича запилена
                        А вот когда фича запилена — вы берёте её и распиливаете на удобные для обсуждения коммиты.

                        Ну блин, вас никто никогда не учил писать сочинения? Черновики и переписывание набело — тысячи лет назад изобретены, почему программисты считают, что к ним это всё не относится и норовят все свои «творческие порывы» влить в репу?
                          0
                          Я вас не понял.
                          Создаю ветку — делаю в ней работу с такими вот «творческими» коммитами, создаю MR на слияние этой ветки с основной (master). Ревьюер видит все мои коммиты.
                          Каким образом я могу сделать по-другому?
                            0
                            Наверное как-то так. Т.е. rebase/pick/squash.
                              0
                              У git'а есть много средств для управления историей. Хотя иногда после недели «творческих изысков» проще просто сесть и с нуля всю историю руками написать.

                              Но change'й без описания — в истории быть не должно (за исключением merge'ей — они могут не описываться если там не происходило никаких ручных правок, просто изменения из двух веток в одну слили).
                              0
                              Каким образом я могу сделать по-другому?
                              Посмотреть на то, что вы натворили и «переписать историю набело»? Если изменение получилось не слишком большое — можно просто всю историю в один commit сжать, если большое — выделяете из вашего изменения атомарные куски, пишите историю до тех пор, пока она вам понравится.

                              Ну собственно как это всегда и делалось. Ну посмотрите же на LKML — оттуда же всё растёт! Что-то типа такого (не ищите глубокого смысла в этом изменение — просто «что под руку попалось»): первый changeset делает то, второй — сё, третий — ешё что-то, после 125го — у нас есть новая готовая фича.

                              Принципиальная идея Git'а — это управление историей. Отсюда всякие штуки типа cherry-pick interactive и прочее.

                              Ревьюер видит все мои коммиты.
                              А нафига они ему? Если вы им даже названия дать не может и сами себе объяснить не можете — что вы там делали, почему и зачем?

                              Кому и в чём они помогут лет через 5? Перед тем, как оправлять ваше изменение на review историю нужно бы подчистить — в это основная идея всех этих review.
                            0
                            На мой взгляд лучше уж многочисленные коммиты типа этих, чем безликие daily commit
                            *** Create models for some feature
                            *** Add UI for some task
                            *** Change feature models structure
                            *** Delete feature.type field
                            *** Update UI for new structure
                              0
                              Да вариант. Но тоже палка о двух концах. В таком творческом процессе постоянная фиксация таких коммитов отвлекает. ИМХО
                                0
                                Если творческий процесс слишком часто приводит к противоречивым коммитам, то стоит попробовать лучше прорабатывать задачи предварительно, хотя бы в своей голове.
                                  0
                                  Зависит от того, насколько оно всё такое из себя творческое.

                                  Я обычно стараюсь минимизировать «масштаб бедствий» и грубоко «в творчество» не уходить.

                                  Поправили чуть интерфейс — давайте от'review'им, за'commit'им, авось не только мне легче будет.

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

                                  А вот когда у тебя цепочка из 20 commit'ов, которые связаны между собой и усложнение интерфейса в 1м упрощает жизнь в 20м… тут да, приходится долго общаться с review'ером и иногда и переделывать…
                              0
                              Лично я в этом случае каждый день делаю коммит «daily commit».

                              Зачем вообще делать коммиты каждый день? Можно доделать задачу и уже по ней сделать один коммит.
                              0
                              Благодарю за интересный и полезный материал. Интересно всё же узнать расшифровку CL которая заменяется на MR в статье. Что это?
                                0
                                Change list

                              Only users with full accounts can post comments. Log in, please.