Код ревью: как быть хорошим автором

  • Tutorial

Привет! Меня зовут Сергей Загурский, я работаю в Joom в команде инфраструктуры. В своей практике ревьюера кода я регулярно сталкиваюсь с тем, что автор не понимает, что ревьюер не является волшебным чёрным ящиком, в который можно закинуть любые изменения и получить по ним обратную связь. Ревьюер, как и автор, будучи человеком, обладает рядом слабостей. И автор должен (если, конечно, он заинтересован в качественном ревью), помочь ревьюеру настолько, насколько это возможно.

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

Зачем мы делаем ревью кода

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

Ревью кода замедляет разработку. Это главный аргумент, который вы услышите от противников ревью. И это правда, но не вся. Ревью действительно замедлит вашу разработку. Изменения действительно будут дольше проходить весь путь от клавиатуры до продакшена. Однако, замедляя разработку «здесь и сейчас», ревью позволяет сделать кривую сложности изменения системы более пологой. Я не претендую на научность, но проиллюстрировал это на графике выше. 

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

На примитивном уровне ревью кода также является моделью того, как с написанным кодом будет идти взаимодействие в дальнейшем. Если у ревьюера не возникает никаких проблем с пониманием, значит и при чтении или отладке кода в будущем также сложностей не будет. И наоборот, если ревьюер не может ничего понять, то это write only код. Его развитие и отладка будут стоить дороже.

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

Дальше мы сосредоточимся на самой фундаментальной составляющей процесса ревью — на понимании кода.

Как ревьюер делает ревью

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

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

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

Бывает такое, что автор вместе с основными изменениями делает что-то, не связанное с решением своей задачи. Когда ревьюер сталкивается с таким, то он тратит дополнительное время, чтобы понять, как эти изменения связаны с решением основной задачи.

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

Также нужно помнить, что ревьюер, будучи человеком, склонен откладывать начало ревью на потом, если видит ревью повышенной сложности. Вежливый ревьюер может постесняться потребовать разбить большой Pull Request на несколько и будет платить за это своим временем и качеством ревью.

Как помочь ревьюеру провести качественное ревью

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

Перед тем, как превратить свои изменения в Pull Request, следует разбить их на логические куски, если в этом есть необходимость. Комфортный объём ревью заканчивается примерно на 500 строках кода изменений. Допустимый — примерно на 1000 строках. Всё, что за пределами 1000 строк, должно быть разбито на более мелкие Pull Request’ы. 

Если вы, как и я, не разбиваете свои изменения на куски комфортного размера заранее, то это придётся проделать перед отправкой ваших изменений на ревью. Отмазка, что на это нужно потратить время, не катит. Принимая решение об отправке на ревью 1000+ строк кода, вы, фактически, оцениваете своё время дороже времени ревьюера. По нашим правилам у ревьюера всегда есть право потребовать разбить изменения на более мелкие куски. Мы всегда просим коллег относиться с пониманием, если он этим воспользуется. С опытом становится проще строить свою работу так, чтобы у вас не появлялись гигантские Pull Request’ы, в которых «ничего нельзя отделить».

Отдельно стоит упомянуть изменения, внесённые в код с помощью авторефакторинга или sed’а. Объём таких изменений может быть очень большим. Наличие автоматических изменений вместе с осмысленными изменениями усложняет ревью. Всегда отделяйте авторефакторинги в отдельные Pull Request’ы, если их объём сопоставим с объёмом нового кода, написанного вами.

Следующая часть подготовки состоит в информировании ревьюера о сути изменений. Как я уже писал выше, ревьюеру нужно знать: что сделано, зачем сделано, как сделано, и почему именно так сделано. Часть информации у ревьюера уже может быть. Часть — есть в коде. Всё, что не очевидно из кода, должно быть прописано в описании изменений. 

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

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

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

В целом, я считаю допустимым потратить порядка 10% от времени, затраченного на написание кода, на подготовку к ревью. Это время автора, которое мы обменяем на экономию времени ревьюера, и на улучшение качества ревью. Следует помнить, что ревьюеру на качественное ревью легко может потребоваться и 20%, и 50% от времени, затраченного автором на написание кода.

Вот только теперь автор может с чистой совестью отправить изменения ревьюеру.

Дальше начинается жизненный цикл Pull Request’а. Ревьюер должен либо одобрить его, либо попросить внести изменения. Чтобы упростить работу ревьюера, автору стоит добавить комментарий к каждому запрошенному изменению. Это вполне может быть краткое «OK» или «Исправил», если ничего другого не требуется. Убедитесь, что вы понимаете, что просит ревьюер и что вам понятна его аргументация. Не стоит безоговорочно принимать любые запросы на внесение изменений, возводя тем самым ревьюера в околобожественный ранг. Ревью — это обоюдный процесс. Если ревьюеру что-то не понятно, то он спрашивает об этом автора, и наоборот. В случае, если автор не очень опытен, ревьюеру следует приложить особенные усилия к описанию запросов на изменения, чтобы воспользоваться возможностью поделиться с автором своим опытом. Бывают и спорные моменты, когда аргументация недостаточно сильная, чтобы склонить обе стороны к единой точке зрения. С учётом того, что автор находится в более уязвимой позиции, считаю, что при прочих равных преимущество должно оставаться за автором.

Не вносите изменений в свой Pull Request, не относящихся к тому, что попросил вас ревьюер. Это крайне сбивает с толку. Также следует воздержаться от rebase и подобных действий.

Получили одобрение от ревьюера? Отлично, ещё одно качественно написанное и оформленное изменение было добавлено в проект!

Joom
Международный мобильный маркетплейс

Комментарии 36

    +2
    Год назад я сам первый раз делал кодревью менее опытному участнику нашей команды и только тогда понял, почему ревьюеры моего кода просили меня разбить пуллреквест на части, делать отдельным ПР форматирование кода и тп. Плакал кровавыми слезами…
      0

      У меня вся команда ревьюит код- помимо остального это помогает им понимать откуда идут правила вида 'коммит недолжен быть большим' и они стараются так неделать.
      Во-вторых при больших пулл реквестах если ты видишь косяк архитектуры и пишешь об этом, то часто все изменить становится больно-буквально пару больших мердж реквестов от джуна в которых видны проблемы дизайна помогают им понимать, что лучше разбивать пр на более мелкие гораздо лучше тысячи объяснений, что так делать нельзя.

        +1

        В нашей компании тоже стараемся задействовать всех коллег в качестве ревьюеров. Но только эта мера не делает из людей хороших авторов, к сожалению. Я регулярно сталкиваюсь, например, с таким отношением к ревью, когда ревьюер говорит "ну мне тут очень сложный код отдали на ревью, поэтому я только нейминг по диагонали проверил". Культуру ревьюеров сформировать, пожалуй, ещё сложнее, чем культуру авторов. Слишком велик соблазн просто подождать два часа и нажать на кнопку "Approved", сделав вид, что ревью проведено. Если коллега ревью делает именно таким способом, то это никак не сделает его хорошим автором.
        Я не утверждаю, что метод совсем не работает. Он ограниченно работает. Но более действенной нахожу обратную связь от ревьюеров. Когда ревьюер раз за разом терпеливо и спокойно объясняет автору, почему нужно разделить большой PR на несколько. Это на масштабе работает лучше, чем "раз ты дал мне PR на 5 тысяч строк, то на, почитай на 6".
        Мы внутренние семинары ещё проводим. Как для авторов, так и для ревьюеров.

          0
          на масштабе работает лучше, чем "раз ты дал мне PR на 5 тысяч строк, то на, почитай на 6".

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


          Я регулярно сталкиваюсь, например, с таким отношением к ревью, когда ревьюер говорит "ну мне тут очень сложный код отдали на ревью, поэтому я только нейминг по диагонали проверил". Слишком велик соблазн просто подождать два часа и нажать на кнопку "Approved", сделав вид, что ревью проведено. Если коллега ревью делает именно таким способом, то это никак не сделает его хорошим автором.

          Честно говоря не могу сказать что сталкиваюсь часто с таким отношением. Причем видел такое в ситуациях только когда код ревью проводится для "галочки"-на любые замечания у автора есть тысяча аругментов почему это надо было сделать именно так и по итогу споров просто ничего не правится.
          У меня хорошо работал вариант когда я кого-либо назначал "главным" по код-ревью, и это были все люди на проекте по очерди-тогда этот человек начинает ревьюить проект вычитавая полностью. Так люди вовлекались в процесс.

            0
            По-моему начинать давать плохие пулл-реквесты "потому что ты такой сам читал"-вообще порочная практика. Это будет замкнутый круг. Лиды должны слать максимально хорошие пулл реквесты, чтобы с них брали пример.

            +100


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

            Если автора три раза попросили поделить большой PR, то в четвёртый раз он его уже поделит сам. Даже если он и не проникся, зачем это нужно. Но я тут не спорю с вашей точкой зрения. Гораздо лучше если автор проникся. И если он сам иногда также является ревьюером, то ему, однозначно, проникнуться будет проще.


            Честно говоря не могу сказать что сталкиваюсь часто с таким отношением.

            Редко кто в таком признаётся, конечно. Но по факту видно же, где ревьюер вчитался, а где шёл по диагонали.


            Причем видел такое в ситуациях только когда код ревью проводится для "галочки"-на любые замечания у автора есть тысяча аругментов почему это надо было сделать именно так и по итогу споров просто ничего не правится.

            Такое тоже видел. Если с автором невозможно нормально спорить, то нужно, чтобы с ним поговорил руководитель и объяснил, что это плохо. Ну а если не сработает, мы все знаем, что с таким автором нужно сделать :)


            У меня хорошо работал вариант когда я кого-либо назначал "главным" по код-ревью, и это были все люди на проекте по очерди-тогда этот человек начинает ревьюить проект вычитавая полностью.

            Интересный подход. Подумаю, можно ли у нас такое применить.

      +1
      Также следует воздержаться от rebase и подобных действий.

      Ну причесать историю перед отдачей на ревью без ребейза не выйдет (выйдет на самом деле, но это не самый лучший способ)


      Бывает что ветка отстала от апстрима так, что мержкоммит стал размером с пуллреквест, такое тоже нельзя отдавать на ревью, как бы вылизан не был пуллреквест.
      Любой нетривиальный конфликт должен быть разрешен разработчиком.


      Причесал, ребейзнул на мастер, причесал ещё раз, отдал на ревью.

        +3
        До ревью — пожалуйста. Во время ревью лучше не стоит редактировать историю, чтобы облегчить труд ревьюера.
          0
          .del
            0

            Как вы конфликты будете резольвить до мержа? Или вы вмержете как-нибудь, а потом перепишете?

          +1
          Прошу прощения за возможно глупый вопрос, а кто какие техники использует для разбиения большого PR на более мелкие, в случае когда саму задачу сложно (невозможно) разбить на более мелкие?

          У самого на проекте очень часто на ревью приходят большие PR и соответственно, их ревьювят «мельком» (грешен, поскольку и сам так делаю).
            +3

            Я применяю два способа, в зависимости от сложности ситуации. Если изменения практически независимы, то их можно отделить так:


            1. "Скинуть" изменения из истории через git reset HEAD~. Если ваши изменения распределены по нескольким коммитам, то вместо HEAD~ должен быть хэш коммита, предшествующего вашим изменениям.
            2. С помощью git add -i или пофайловым git add или любыми аналогами, которые у вас под рукой, добавить изменения в коммит. Закоммитить.
            3. Повторить пункт 2 для каждого отдельного изменения.

            Второй способ позволяет отделять изменения, которые тесно связаны:


            1. Сохранить на всякий случай исходные коммиты в каком-нибудь отдельном бранче. Можно, в принципе, и просто где-то хэш коммита записать. Если что-то пойдёт не так, всегда можно будет начать сначала.
            2. "Скинуть" изменения так же, как в пункте 1 из предыдущего способа.
            3. Идём от обратного, удаляем изменения, которые не должны попасть в первую порцию изменений. Делаем это любым подходящим для вас способом. Часто это будет ваша любимая IDE.
            4. Получили первую порцию изменений без остального. Делаем git add ... и git commit.
            5. Возвращаем исходные изменения в рабочую копию через git checkout <хэш-из-п1> ..
            6. Можем повторить шаги, начиная с 3, чтобы отделить ещё изменения.

            Разумеется, это всё не какие-то волшебные способы, которые за вас могут отделить изменения за 10 секунд. Потребуется провести дополнительную работу. Обычно, это не занимает много времени, относительно времени, затраченного на написание исходных изменений. Но и удивляться не стоит, что на распутывание клубка из 3K+ строк и 10 связанных изменений нужно 2 часа. Это время окупается при ревью как временем ревьюера, так и качеством ревью.

              +2

              Кажется вопрос не о том. Вопрос про декомпозицию задачи, когда задача большая и монолитная. Но тут никакие инструменты не помогут. Если таких задач много, то стоит пересмотреть куда более фундаментальные процессы в команде.

                0
                Возможно не совсем правильно сформулировал вопрос, но я имел в виду именно разбиение PR на уровне гита.

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

                  0
                  в случае когда саму задачу сложно (невозможно) разбить на более мелкие

                  Я на эту фразу опирался. Гит же позволяет вам хоть по одной строчке коммитить. Переписать историю можно с помощью interactive rebase.

                    0

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

                      0
                      Вы описали разбиение на коммиты, но разбиение на коммиты не имеет никакого смысла.
                      Ревьювятся не коммиты, а результирующий дифф между веткой и мастером.

                      Как это, никакого смысла? Если всё в одном коммите, то как сделать разные Pull Request'ы для ревью?


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

                        0

                        В смысле "не имеет смысла в одной ветке".


                        Разные пулл-реквесты должны быть из разных веток.

                          0

                          Это в вашем процессе не имеет смысла, а в других — вполне имеет.


                          В Linux, например, ревьювятся отдельные коммиты из ветки. Вернее, набор коммитов.

                            0
                            > В Linux, например

                            Что такое Linux?

                            > ревьювятся отдельные коммиты из ветки

                            Где вы почерпнули эти сведения? Ссылка есть?
                              +1

                              Ядро операционной системы. Можете читать любой из их списков рассылки в поисках показательных примеров процесса разработки. И документацию на процесс.


                              Вот первый попавшийся набор патчей из LMKL, в котором есть дискуссия:
                              pragma once: treewide conversion
                              Набор из 11 патчей. Принимается или отклоняется набор в целом. Но каждый из патчей обсуждается отдельно, а не в виде мегадиффа. И при этом они не отправлены в виде 11 не связанных между собой патчей.

                            0
                            В смысле "не имеет смысла в одной ветке".

                            Если это вы меня процитировали, то я не вижу, чтобы я такое писал. Когда коммит разбивается на несколько, то изначально они в одной ветке находятся. Потом можно растащить по разным.


                            Разные пулл-реквесты должны быть из разных веток.

                            Вы абсолютно правы и я, вроде бы, нигде не писал обратного.

                  0

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


                  Пример: необходимо реализовать большую фичу, чтобы ее реализовать необходимо выполнить следующие шаги:


                  1. шаг 1 (отрефакторить)
                  2. шаг 2 (добавить новую мелкую фичу)
                  3. шаг 3 (реализовать большую фичу)

                  Ветка с большой фичей будет выглядеть следующим образом:


                  1. merge "шаг 1" into "большая фича"
                  2. merge "шаг 2" into "большая фича"
                  3. merge "шаг 3" into "большая фича"
                  4. commit 1
                  5. ...
                  6. commit n

                  Смысл в том, что крупные шаги можно отследить по merge commit'ам.

                  0
                  потратить порядка 10% от времени, затраченного на написание кода, на подготовку к ревью. Следует помнить, что ревьюеру на качественное ревью легко может потребоваться и 20%, и 50% от времени, затраченного автором на написание кода.

                  А сколько при таком раскладе будет уходить на исправление замечаний по ревью?
                  т.е. 10% подготовка, 20-50% само ревью. Если есть замечания, то сколько в среднем закладывается на них?
                  Мне кажется это ключевое в процессе. Т.е. убедить менеджмент что вы будете заниматься какой-то неизмеряемой деятельностью 30-60% процентов времени.
                    +1

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

                      0
                      А у вас какая-то внутренняя разработка или заказная? ну т.е. кто-нибудь занимается оптимизацией процесса(чтобы сделать быстрее\дешевле чем конкуренты) или просто есть внутренний бюджет и вы его расходуете?
                        +1
                        ну т.е. кто-нибудь занимается оптимизацией процесса(чтобы сделать быстрее\дешевле чем конкуренты) или просто есть внутренний бюджет и вы его расходуете?

                        Мы стараемся оптимизировать процессы, чтобы внесение изменений обходилось дешевле. Проект у нас "бесконечный", мы его будем развивать столько времени, сколько позволит бизнес. Поэтому, мы заинтересованы мыслить стратегически, заглядывая и на год, и на два, и на пять лет вперёд.


                        Не очень понял про "внутренний бюджет", т. к., очевидно, он у нас, в широком смысле, есть. Как и у всех.

                          0
                          так а почему если оптимизируете, то оставляете процесс который занимает 30-60% времени и дает не очень хорошие результаты(т.е. нет реджектов кода или их мало).
                          Ну т.е. я понимаю что для программистов это наверное хорошо, но почему менеджмент это не присекает
                            0
                            дает не очень хорошие результаты(т.е. нет реджектов кода или их мало)

                            Ревью даёт прекрасные результаты! Малое количество реджектов это следствие наличия процесса ревью. Если бы ревью не было, то качество кода было бы ниже.


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

                    0
                    Отличный пост, спасибо! У нас еще часто ведутся споры насчет критериев нужности PR. Где-то принято на каждое изменение создавать Pull Request, где-то вообще их не создают, коммитят/мержат прям в основную ветку. Есть ли у вас какие-то формальные критерии, по которым определяете, нужно ли создавать Pull Request или можно коммитить и так?

                    остатки отладочных конструкций, закомментированного кода, TODO-комментариев, бессмысленных изменений форматирования, не пригодившегося кода и прочего подобного
                    По-моему, имеет смысл делать это перед каждым коммитом, не стоит откладывать до создания PR, где таких мусорных изменений может быть очень много. Ну и современные IDE имеют кучу встроенных pre-commit проверок, которые сильно упрощают эту задачу. Можно конечно последним коммитом делать эдакий «cleanup», но мне кажется проще коммитить уже чистый код.
                      +1
                      Отличный пост, спасибо!

                      Спасибо!


                      Есть ли у вас какие-то формальные критерии, по которым определяете, нужно ли создавать Pull Request или можно коммитить и так?

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


                      По-моему, имеет смысл делать это перед каждым коммитом

                      Если автор так делает, то автор — молодец! Не шучу. Я так не умею и всегда все хвосты подчищаю в самом конце. Я слышал также, что существуют люди, которые заранее всё правильно нарезают на коммиты. И без багов пишут!

                        0

                        Критерия "нужно ли создавать пулл-реквест" тут быть не может. Тут должно быть железное правило: никаких коммитов в мастер, все изменения — только через пулл-реквесты, всегда, без исключений.


                        Потому что любое "тривиальное" изменение может оказаться вовсе не тривиальным.

                          0
                          Критерия "нужно ли создавать пулл-реквест" тут быть не может. Тут должно быть железное правило: никаких коммитов в мастер, все изменения — только через пулл-реквесты, всегда, без исключений.

                          Вы чересчур категоричны :)


                          Это зависит от требуемого баланса скорости разработки и стабильности продукта. Иногда бывает норм вообще не ревьюить, сразу в прод фигачить. А иногда — через три комиссии любое, сколь угодно тривиальное, изменение проводить.

                            0
                            > Иногда бывает норм вообще не ревьюить, сразу в прод фигачить.

                            Только если это ваш личный проект и вы там один участник.

                            В прочих случаях нельзя формализовать вот это «бывает норм» — вам норм, а другому участнику не норм. И чтобы не было никаких разночтений и бардака — всё должно быть по единой схеме.
                              0
                              Только если это ваш личный проект и вы там один участник.

                              Ну не только, конечно. Есть и другие кейсы. Например, стартап в фазе PoC. Например, проект с ограниченным сроком жизни, вроде программного обеспечения к какому-нибудь уникальному ивенту или промосайта. Уверен, этими примерами список не исчерпывается. Загоняя себя в такие категоричные рамки, вы отбираете у себя возможность гибко регулировать баланс скорость-качество.

                        0
                        Спасибо за хорошую статью. Предлагаю ещё не забывать про статический анализ. Я понимаю, что у меня профессиональная деформация на эту тему, но мне кажется важным добавить следующую мысль. Помимо всего прочего, быть хорошим автором – означает применять инструменты статического анализа до начала Code Review. Иначе есть риск, что внимание коллег смещается в сторону поиска опечаток, а не к обзору алгоритмов, красоте именования переменных, понятности кода и так далее.

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

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

                        Взгляните, например, на этот код:
                        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. И опять-таки на эту тему есть целая статья: Зло живёт в функциях сравнения.

                        Итак, при обзорах кода программисты часто не замечают ошибки. Что с этим делать? Использовать вспомогательные инструменты, такие как статические анализаторы кода. Эти программы, в отличие от человека, не устают и не ленятся :)
                          +1
                          Спасибо за хорошую статью.

                          Спасибо!


                          Предлагаю ещё не забывать про статический анализ. <...>

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

                        Только полноправные пользователи могут оставлять комментарии. Войдите, пожалуйста.

                        Самое читаемое