Что я узнал после более чем 1000 code review

Original author: Juraj Malenica
  • Translation
За последние 3 года я рассмотрел более 1000 pull (merge) request’ов. За это время я многому научился — в основном тому, как не проверять код, как сделать процесс менее болезненным, что делает код хорошего качества и так далее.

Pull request должен делать только одну вещь


Это самая важная вещь, на которую стоит обратить внимание.

Делая code review, вы должны держать в голове много вещей. «Что за этим стоит?», «Как это согласуется с остальной частью кода?» и «Будет ли это хорошо работать?» Вот лишь некоторые из вопросов, на которые нужно ответить. Таким образом, когда у вас есть pull request, который пытается решить одну проблему, на некоторые из этих вопросов легче ответить.

Другим важным аспектом является размер pull request’а. Большие запросы требуют экспоненциально больше времени для рассмотрения. И когда я узнаю, что мне нужно потратить более 15 минут на запрос, вам придется подождать до пары часов.

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

Лучше всего разбить pull request’ы на осмысленные части — один запрос должен решить только одну вещь.

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

Автоматизируйте как можно больше проверок


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

Хорошо, что есть довольно простое решение — интегрируйте все проверки в CI. Тогда у вас будут запускаться все проверки всякий раз, когда кто-то отправляет pull request, и не разрешает слияние пока все проверки не пройдут успешно. Вам как рецензенту больше никогда не придется беспокоиться о форматировании.

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

Автоматическое форматирование кода приводит к тому, что все споры вокруг идеальной длины строки или места добавления новой строки исчезают. Просто решите командой какой набор правил форматирования вам подходит и передайте его автоформатору. Это избавит вас от многих неприятностей. Если у вас возникли проблемы с согласованием формата, посмотрите, как это делают Google, Facebook или другие крупные компании.

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

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

Сядьте с автором, чтобы просмотреть код


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

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

Будьте осторожны, чтобы не переусердствовать, хотя. Некоторые pull request’ы слишком малы и слишком просты, чтобы вести какой-либо эффектный разговор. В этом случае встреча с автором может быть пустой тратой времени.

Будьте внимательны при написании отзывов


Если вы более опытны, чем автор кода, вы должны принять во внимание, что то, как вы говорите, имеет большое значение. Хорошо написанная критика может помочь разработчику стать лучше в будущем, но она также может разрушить чьи-то мечты.

Я обнаружил, что лучше всего задавать открытые вопросы — это не агрессивно и даже побуждает разработчика мыслить критически. Это займет больше времени, чем просто рассказать кому-нибудь решение? Да, краткосрочно. Но в долгосрочной перспективе вы помогаете им расти, и они с меньшей вероятностью повторят свои ошибки.

Итак, в следующий раз, когда кто-то откроет файл внутри цикла for, а не перед ним, вместо того, чтобы просто указать на это, спросите: «Как бы вы уменьшили сложность здесь?». Это будет много значить.

Добавьте флаг «Я запускал этот код локально»


Больше всего меня беспокоит обнаружение ошибки в небольшом запросе, из-за которого функциональность вообще не работает. Это означает, что разработчик даже не запустил код — вероятно, думая, что в этом нет необходимости, поскольку изменения настолько малы.
После того, как это случилось пару раз, я добавил флаг под названием «Я запускал этот код локально», что полностью решило проблему. Я прекратил просмотр кода, который не запускался локально, и люди не лгали об этом. Не было обид, потому что мы все знали, что это нужно делать.

Бонус: это наш шаблон, который должен заполнить каждый разработчик при создании pull request’a:

Описание Merge Request’a
Что нового?

Реализовано…
Что изменилось?

Изменено…
ЧЕК-ЛИСТ
[] Я запускал этот код локально
[] Я написал необходимые тесты
[] Я покрыл свой код тайп хинтами
[] Я обновил CHANGELOG
Трелло карта
trello.com/c

Должен знать о
Есть что-нибудь еще, что должно быть известно?
Любые замечания по развертыванию?
Любая дополнительная документация?


image

Узнайте подробности, как получить востребованную профессию с нуля или Level Up по навыкам и зарплате, пройдя платные онлайн-курсы SkillFactory:



Читать еще


SkillFactory
Онлайн-школа Data Science и разработки

Comments 49

    +21
    западная толерантность
    Если вы более опытны, чем автор кода, вы должны принять во внимание, что то, как вы говорите, имеет большое значение. Хорошо написанная критика может помочь разработчику стать лучше в будущем, но она также может разрушить чьи-то мечты.

    Я обнаружил, что лучше всего задавать открытые вопросы — это не агрессивно и даже побуждает разработчика мыслить критически. Это займет больше времени, чем просто рассказать кому-нибудь решение? Да, краткосрочно. Но в долгосрочной перспективе вы помогаете им расти, и они с меньшей вероятностью повторят свои ошибки.

    Итак, в следующий раз, когда кто-то откроет файл внутри цикла for, а не перед ним, вместо того, чтобы просто указать на это, спросите: «Как бы вы уменьшили сложность здесь?». Это будет много значить.
    Вот всем приятно работать с западными заказчиками. Кроме вот этих «задвигов», прости г-споди.
    Фидбак адекватный получить нереально.
    Сдаешь выполненное ТЗ, вплоть до буквы, и начинается «что-то мне не нравится, как Вы думаете что можно было бы улучшить» и иди блин 3 дня размышляй в попытках угадать что заказчику не понравилось.
    Вроде найдешь то, чем думаешь проблема, ускоришь загрузку сайта в 2 раза, а он такой разочарованно «ну вот, ничего не поменялось» и после еще 3 суток попыток выяснить в чем дело уходит жалоба выше вроде «плохой работник, уже неделю не может поменять цвет иконки»©

    Ну ладно, это лирика. К конкретике.
    Вопрос «как бы Вы уменьшили сложность тут» ужасен по 2 причинам.
    1) Это вопрос в стиле «Вы перестали пить коньяк по утрам». Почему? Потому что ревьюер не зная причин такого решения уже заведомо решил что это решение неправильное, даже не интересуясь причинами.
    2) Этот вопрос не дает даже примерно информации о проблеме о которой говорит ревьюер. Как правило, если кодер что-то сделал, он это сделал не потому что он саботирует проект, а потому считает что это правильно. Поэтому с хорошей степенью вероятности он не поймет о чем именно речь и в лучшем случае потеряет тучу времени в догадках, заодно убеждаясь в своей непроходимой тупости — что он не может сразу сходу найти проблему.

    Правильно было бы спросить «по каким причинам Вы выбрали Вы открываете файл в цикле? не считаете ли Вы что это неадекватно увеличит сложность?».
    В такой постановке во-первых, сразу идет разговор по делу и прогер не занимается гаданием; во-вторых, ревьюер оставляет за собой право на ошибку.
      0
      Сдается мне, что вся эта «западная толерантность» не на пустом месте. Не знаю с чем связанно, но напрямую указать на проблему без открытой агрессии и экспрессии они не могут. Т.е. у них нет опции просто сказать «пива нет», а только либо пафосно «Пива-а не-е-е-т!!! » либо толерантно поставить открытый вопрос
        +1
        Я думаю вы правы. Естественно за всех говорить нельзя. Но вот улыбки так же не на пустом месте взялись. Видимо, вынужденная мера чтобы все не перегрызлись.
          0

          Или более того — чтобы не перестреляли друг друга во времена Дикого Запада. Наверно, чаще выживали те, кто сразу давал понять, что настроен мирно, что не представляет опасности.

          +1

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


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

            +1
            Мальчику стажеру я когда-то на пул реквестор в 20 строк написал много замечаний и рекомендаций прикрепив примеры и вышел за рамки экрана, а так же использовал такое:
            Так не правильно, потому-то
            Это не сработает, так как
            Это сложно/Зачем это
            Найди другой способ решения согласующийся с

            Человек испугался :(

            Вообще не важно человек писал код 5 минут или час или скопировал где. Как только пишешь, что мол код негодный и вот места с вопросами — мотивация у него падает и ещё агресия встречается.
            Люди сами не могут код от себя отделить.
            Люди не могут просто так отказаться от своей идеи.
            Люди не хотят тратить время на переделывание, «так как делают по максимуму», а это «больше придирки и вкусовщина» и даже жалуются на выгорание и отсутствие мотивации.
            Часто получается так, что они хотят ПР пройти, а не код написать.
              0
              Мальчику стажеру я когда-то на пул реквестор в 20 строк написал много замечаний и рекомендаций прикрепив примеры и вышел за рамки экрана, а так же использовал такое:
              Так не правильно, потому-то
              Это не сработает, так как
              Это сложно/Зачем это
              Найди другой способ решения согласующийся с

              Человек испугался :(
              Мы бы благодарны были, разве что в ноги не поклонились бы. На курсах блин отваливаешь по косарю в месяц что бы препод хотя пару раз в неделю говорил что ты не так делаешь по паре пунктов, а тут подробнейший ревью по делу от практикующего специалиста и нос воротить от такого?

              Люди сами не могут код от себя отделить.
              Люди не могут просто так отказаться от своей идеи.
              И что они делают в программерах? В профессии, когда обучение никогда не заканчивается по определению?
                0

                Мой опыт: Я с русскоговорящими в том числе работаю. И казалось бы такая «закалка» и «конструктивная» критика, которая даётся в СНГ должна давать преимущество.
                Но я все время сталкиваюсь с такой проблемой, что люди не готовы озвучивать свои мысли.
                «Я это сделаю. Я все понял» через неделю заканчивается тонной велосипедного кода.
                А в начале же говоришь, что можно созвониться или початиться или мыло отправить, если есть вопросы или недопонимание. Но нет.
                А помощь нужна? Конечно же нет.


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

                  0
                  Вы правильно сказали что нужно расписать свои мысли обязательно подкрепляя «потому что ...». Далее надо начать дискуссию с обязательным участием обоих сторон. Чтобы не я против тебя, а чтобы мы против проблемы. Психологически это воспринимается гораздо легче и вы скорее всего поймете что на уме у собеседника и сможете направить его в нужную сторону.
            +3
            Поэтому с хорошей степенью вероятности он не поймет о чем именно речь и в лучшем случае потеряет тучу времени в догадках, заодно убеждаясь в своей непроходимой тупости — что он не может сразу сходу найти проблему.
            С другой стороны, сталкивался с такой проблемой — объясняешь человеку суть за 5 минут, он все понимает, но тут же забывает и в следующий раз опять спрашивает. Но если заставить его думать, то найдя решение через час, он запоминает его надолго.
            Конечно подход не универсальный и не для всех задач, но те решения до которых додумался самостоятельно запоминаются намного прочней.
              +3
              Думаю все-таки очень важно следить за лексиконом во время код-ревью. Даже такие слова как «неадекватно» могут выглядеть чересчур резкими. Помните, что вы и без этого в позиции силы, аппрувал кода зависит от вас и нет нужды дополнительно демонстрировать эту силу.

              Я бы переформулировал эту фразу более нейтрально: «Этот файл открывается в цикле, это может сильно сказаться на сложности/производительности кода». Желательно дополнить это еще советом (но не указанием, автор может придумать что-то лучше чем вы) о том, как этот код можно улучшить. Например: «Попробуйте переписать код так, чтобы открыть файл один раз перед циклом и переиспользовать дескриптор»
                +1
                Думаю все-таки очень важно следить за лексиконом во время код-ревью. Даже такие слова как «неадекватно» могут выглядеть чересчур резкими. Помните, что вы и без этого в позиции силы, аппрувал кода зависит от вас и нет нужды дополнительно демонстрировать эту силу.
                Вот это и есть проблема с чрезмерной толерантностью.

                Во-первых, термин «неадекватно» (применительно к коду/решению) это всего лишь «не соответствующий чему-либо, неподходящий для чего-либо». Таким образом когда Вы называете это слово слишком резким — Вы ставите субъективное мнение выше объективного значения и тем самым открываете врата ада. Когда кодер должен гадать не только о том, где он сделал что-то не так, но и еще гадать как бы ему ответить так, что бы это не звучало для ревьюера слишком резко с точки зрения субъективного мнения ревьюера. Подобная атмосфера не способствует взаимопониманию и продуктивности.

                Во-вторых, использование Вами фразы «с позиции силы» предполагает что в коллективе нет здоровых, партнерских отношений и взаимопомощи, а есть отношения подавления и силы и hostile environment (в голову не приходит как сказать это по русски, извините). И проблема именно в этом, использование терминологии может маскировать эту проблему, но никак ее не решит. В коллективе должна быть такая атмосфера, что бы наличие позиции силы у вышестоящего не подразумевалось, тогда любые замечания будут восприниматься в позитивном ключе.

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

                  Подобная атмосфера не способствует взаимопониманию и продуктивности.

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

                  есть отношения подавления и силы и hostile environment

                  Быть в позиции силы не означает, что ее нужно использовать, но все же у ревьюера она есть, и это факт, о котором стоит помнить.

                  тогда любые замечания будут восприниматься в позитивном ключе

                  Не могу согласиться со словом «любые», опять-таки это слишком субъективно и нужно помнить что разные люди воспринимают жесткую критику по-разному.

                  P.S.
                  Вот это и есть проблема с чрезмерной толерантностью.

                  Чисто из интереса: считаете ли фразу, которые я предложил как коменты в код-ревью излишне толерантными? Такой же вопрос к общему стилю моих комментариев в этом треде.
                    –1
                    Быть в позиции силы не означает, что ее нужно использовать, но все же у ревьюера она есть, и это факт, о котором стоит помнить.
                    Наличие у ревьюера позиции силы, имхо, означает неправильную огранизацию воркфлоу. Достаточно кодеру дать прав апелляции и позиция силы либо нейтрализуется, либо оказывается у кодера (в случае если ревьюеру прилетает за его ошибки сильнее чем кодеру).
                    Возможность «пожаловаться» должна быть абсолютно штатной и должна уравновешивать «силы». И это даже вопрос не толератности, а продуктивности.

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

                    Чисто из интереса: считаете ли фразу, которые я предложил как коменты в код-ревью излишне толерантными? Такой же вопрос к общему стилю моих комментариев в этом треде.
                    Излишне толерантными — нет, вот вообще ни разу, если отвечать строго на этот вопрос.
              +9
              Будьте осторожны, чтобы не переусердствовать, хотя.

              Ну нельзя же настолько откровенный гуглоперевод публиковать!

                0
                пока думал как бы об этом в стиле статьи вокруг да около написать — опередили:)
              0
              Вставлю свои пару центов:
              1. Pull request должен делать только одну вещь
                Иногда бывает так, что в коде живет баг и все работает, но именно ваш PR с ним работать не будет. Вариант: бить на 2 PR, причем новый PR с баг фиксом нужно обосновать, согласовать и опубликовать, что задерживает реализацию основного PR от недели до бесконечности (в зависимости от...).
              2. Сядьте с автором, чтобы просмотреть код
                Сейчас, судя по статьям здесь же, все работают на удаленке, хотя расшарить экран конечно же можно, если вы в близких тайм-зонах и интернет шустрый.
              3. Добавьте флаг «Я запускал этот код локально»
                Ну это поддерживаю двумя руками, после некоторых мелких изменений, код не то что не запускается но даже не компилится) Причем желательно уточнить на чем и на какой версии hardware запускали. Возможно какие-то переменные окружения, логины, пароли итд.
              4. Ну и удобно бить крупный PR на мелкие комиты с информационными комментариями. Слишком часто в коментах попадается «minor change» и все. За такие коменты предлагаю бить по рукам).

                0
                >Pull request должен делать только одну вещь
                Тут проблема в том, что этот совет неконструктивен. Чтобы его выполнить, нужно определить, что такое эта «одна вещь». Собственно, вы это почти и иллюстрировали своим комментом.

                Кроме того, разбить PR на несколько мелких — значит получить в N раз больше PR, которые непонятно как связаны друг с другом (потому что проследить эту связь между разными PR весьма проблематично). Такое лечение вполне может оказаться хуже болезни.

                >Добавьте флаг «Я запускал этот код локально»
                Во многих моих проектах код просто локально не запускается. Например потому, что работает в среде Hadoop — а локальные имитации кластера это просто убожество. Я понимаю, почему эту идею поддерживают, но она далеко не универсальна.
                  +1

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


                  Наверное здесь "локально" означает "в тестовой среде" она может быть нелокально, а в облаке, просто код в тестовом инстансе развернут из данного конкретного бранча. Судя по тексту, автор имел ввиду "я провел ручное End-To-End тестирование этого кода"

                    0
                    Ну, проблема в том, что авторы большинства таких статей не понимают (или что еще хуже — скрывают) факт неуниверсальности своих советов. Поэтому советы по большей части получаются из серии «лучше быть богатым и здоровым».
                      +1
                      автор данной статьи просто написал что то в «свой бложик» для новичков
                      а переводчику вообще пофиг что переводить, лишь бы продвигать свои курсы
                        0
                        Согласен
                    0
                    Я для себя давно пришёл к выводу, что ветки надо создавать не по фичам, а по разработчикам. Это прям снимает кучу проблем.
                      0
                      А одну фичу при этом делает сколько людей?
                        0
                        Смотря что вы подразумеваете под фичей. Если таску в джире, то её делает один человек. И один пул реквест покрывает потенциально несколько тасок.

                        В общем читать здесь: www.atlassian.com/git/tutorials/comparing-workflows/forking-workflow
                          0
                          Не очень понятно, какую именно кучу проблем этот подход снимает, и почему не создает другие.

                          Ну ок, один человек ведет свою работу в своем репозитории. И делает несколько задач.

                          Понятно что у него-то все будет хорошо — пока не нужно будет эти таски объединять с чужими в рамках одного релиза. Но в этот-то момент что помешает возникновению конфликтов, например?
                            0
                            Потому что мы мержим конкретный код у которого есть конкретный автор, а не просто кучу абстрактных бренчей, которые мы создаём тысячами.
                            Автор всегда имеет удалённую копию и может синхронизировать свою работу.
                            Всегда можно посмотреть кто над чем работает.
                            Работа разработчика не блокируется, если пул реквест отсрочен. Он может продолжать мерджить из мастера и продолжать свою работу.
                            И такси надо объединять не в рамках релиза, а постоянно.
                              0

                              Вы имеете ввиду что на одного разработчика ровно одна ветка? Или что ветка на фичу И разработчика?


                              Ссылка на forking workflow она про создание форков репы а не про ветки.


                              Если разработчик заводит ветку на инкремент (багу или новую фичу) то все плюсы, которые у вас упомянуты уже соблюдаются.


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

                                0
                                Один разработчик — одна ветка. Заводить по ветке на фичу — крайне неудачная идея по моему опыту.

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

                                Forking workflow же вообще не имеет никаких недостатков и выглядит более элегантно.

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

                                  Что такое "правильно разбивать" и почему их невозможно разбить?


                                  Кроме того разработчики часто работают над несколькими задачами одновременно. Надо постоянно переключаться между ветками.

                                  Что их заставляет так делать? Как они поступают когда одна задача готова, а другая — еще нет? Как потом в истории разобраться зачем именно было сделано конкретное изменение?


                                  Непонятно куда девать коммиты которые не связаны непосредственно с тасками, либо таски не созданы, либо уже закрыты.

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


                                  С моей точки зрения, все коммиты связаны с задачами, если у вас нет никакой задачи не создавайте коммит. Если у вас есть задача, оформите это в багтрекере. Если у вас коммит связан с закрытой задачей, значит в багтрекере вранье — либо задача не закрыта, либо это новая задача, связанная с данной.

                                    0
                                    Что их заставляет так делать? Как они поступают когда одна задача готова, а другая — еще нет? Как потом в истории разобраться зачем именно было сделано конкретное изменение?


                                    Что значит зачем? Программирование — это творческий процесс. Разработчики работают, как им удобнее. Если у меня есть 2 задачи по фронту и бакенду для фичи A и фронт+бакэнд для фичи B.
                                    То мне может показаться удобнее написать сначала бакэнд для обеих фич а затем фронт для обеиз фич.

                                    Как потом в истории разобраться зачем именно было сделано конкретное изменение?


                                    В коммитах должно быть описание. Ваши ветки в этом, кстати, мало помогают. Ибо разработчики смотрят историю основной ветки (как правило).

                                    С моей точки зрения, все коммиты связаны с задачами, если у вас нет никакой задачи не создавайте коммит. Если у вас есть задача, оформите это в багтрекере. Если у вас коммит связан с закрытой задачей, значит в багтрекере вранье — либо задача не закрыта, либо это новая задача, связанная с данной.


                                    То что вы описываете-это самая настоящая бюрократия. И она плохо ложится на реалии.
                                    Я например постоянно переписываю документацию. И что мне для каждой опечатки таску и отдельную ветку создавать?
                                      0
                                      Что значит зачем? Программирование — это творческий процесс. Разработчики работают, как им удобнее. Если у меня есть 2 задачи по фронту и бакенду для фичи A и фронт+бакэнд для фичи B.
                                      То мне может показаться удобнее написать сначала бакэнд для обеих фич а затем фронт для обеиз фич.

                                      Ок тут согласен — вполне допустимо в каких-то случаях создавать ветку на несколько связанных задач, если их удобнее сделать как одну.


                                      Но вы-то предлагает ветку на разработчика — т.е. там должны быть вообще все зачачи разработчика даже логически не связанные или как?


                                      В коммитах должно быть описание. Ваши ветки в этом, кстати, мало помогают. Ибо разработчики смотрят историю основной ветки (как правило).

                                      Наверное тут какое-то недопонимание. Я подразумеваю, что ветки сливаются в какой-то момент в основную. Соответсвтенно, merge commit будет содержать описание и ссылку на задачу и можно будет посмотреть в истории по master.


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


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

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

                                        0
                                        Но вы-то предлагает ветку на разработчика — т.е. там должны быть вообще все зачачи разработчика даже логически не связанные или как?


                                        Совершенно верно. Собственно так это и задумывалось: youtu.be/4XpnKHJAok8?t=1599

                                        Я подразумеваю, что ветки сливаются в какой-то момент в основную. Соответсвтенно, merge commit будет содержать описание и ссылку на задачу и можно будет посмотреть в истории по master.


                                        Merge commit будет содержать название ветки, но что мне это даст? Не проще ли просто посмотреть в комментарии к коммитам.

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


                                        Как фича бренчи улучшают понимание, если разработчики синхронизируются только с мастер веткой? Они могут появляться и исчезать без всяких следов. Так зачем их создавать тогда?

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

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

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

                                          Приведите цитату плиз, я там услышал что им удобно репо на команду, а не ветку на разраба


                                          Merge commit будет содержать название ветки, но что мне это даст?

                                          Обычно он содержит ссылку на PR которую можно открыть и посмотреть все детали


                                          Как фича бренчи улучшают понимание, если разработчики синхронизируются только с мастер веткой?

                                          Если они синхронизируются с мастером, то, по крайней мере, можно для работе в процессе понять какие изменения для какой фичи предназначены. Можно так же мерджить их в мастер пофично (всегда решить какую фичу вмерджить, а какую — нет). Для готовой работы есть уже мастер с его историей.


                                          Но зачем засорять гит тысячами мелких бренчей мне непонятно.

                                          Так они его не засоряют. Обычно они живут пока фича разрабатывается и умирают после мерджа в мастер — причем, это автоматически происходит при завершении пулл реквеста. При следовании соглашениям о наименовании для бранчей типа feature/AddLoginDialog их можно не видеть, пока явно не захочешь, но зато, если захочешь можно увидеть.

                                            0
                                            Обычно он содержит ссылку на PR которую можно открыть и посмотреть все детали


                                            Почему нельзя взглянуть сразу в коммит сообщения, где так же будет ссылка на таску в джире (если коммит связан с таской). Зачем мне идти в мердж коммит, который фиг знает где, открывать пул реквест и только там смотреть конкретные таски?

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


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

                                              0
                                              Почему нельзя взглянуть сразу в коммит сообщения, где так же будет ссылка на таску в джире (если коммит связан с таской). Зачем мне идти в мердж коммит, который фиг знает где, открывать пул реквест и только там смотреть конкретные таски?

                                              Что значит фиг знает где? Он составляет историю мастера.


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


                                              Если она одна, наверное, иметь ссылку на PR чуть менее удобно, чем на фичу в джире на один дополнительный клик. Зато имея ссылку на PR можно посмотреть и дополнительные вещи (типа кто ревьюил и какие были замечания.)


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


                                              Зачем что-то мержить в мастер пофично?

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


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

                                              Если она не вмерджена в мастер, значит не готова. Берите всегда из мастера. (Кроме того случая, когда вы работаете над долгоиграющей фичей — тогда берите из фичи, над которой работаете)

                                                0
                                                Что значит фиг знает где? Он составляет историю мастера.


                                                Вы пытаетесь использовать ветки для документации истории изменений. Но они для этого не предназначены. Они нужны для изоляции кода.
                                                Для документации изменений существуют комментарии к коммитам, где может быть любая исчерпывающая информация. Мердж коммит находится «вдалеке» от основного коммита и надо искать где находится тот, про который написано в мердже.

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


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

                                                Если она не вмерджена в мастер, значит не готова. Берите всегда из мастера.


                                                Необязательно: она может быть сделана, но непроревьювена.
                                                  0
                                                  А как быть если на ревью развернули одну фичу из пакета? Получается все фичи, в том числе не связанные, встают из-за одной. И если вы не придерживаетесь принципа «не сделано — пока не прошло ревью», то вы можете отнаследоваться от кода, который потребует глубокой переработки и сделать много лишней работы.
                                                    0
                                                    Вы пытаетесь использовать ветки для документации истории изменений. Но они для этого не предназначены. Они нужны для изоляции кода.

                                                    Как раз для изоляции — фич друг от друга.


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

                                                    Коммиты бывают разные — например в процессе разработки фичи могут быть эксперименты, откаты на предыдущие версии, мердж с мастером и так далее.


                                                    Все это нужно до того момента, когда надо интегрировать фичу, после того как фича готова, это можно засквешить.


                                                    Мердж коммит находится «вдалеке» от основного коммита и надо искать где находится тот, про который написано в мердже.

                                                    Делается squash merge и я вижу только один коммит. Если не хочется сквешить, то можно проребейзить перед мерджем и они будут рядом.


                                                    А в мастер не обязательно мержить, когда вся фича готова. Можно замерджить и 10 фич и полфичи.

                                                    Чем больше объем мерджа тем больше разбираться что сломало CI, меньше гранулярность при черрипике, если надо будет, больше объем ревью и так далее.


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


                                                    Необязательно: она может быть сделана, но непроревьювена.

                                                    Зачем ее тогда брать, после ревью может быть сильно изменена?


                                                    Я бы сказал, что это нетипичный случай.

                                                      0
                                                      Как раз для изоляции — фич друг от друга.


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

                                                      Даже если допустить что мерж коммиты дают кое-какую дополнительную информацию, то на практике я не припомню, чтобы я когда-либо с этим заморачивался. Обычно я смотрю на конкретные коммиты.

                                                      Чем больше объем мерджа тем больше разбираться что сломало CI, меньше гранулярность при черрипике, если надо будет, больше объем ревью и так далее.


                                                      Ну вообще это больше зависит от пропускной способности ревьюверов, а не от бренчинг модели.

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


                                      А в случае с одной веткой на разраба как эта проблема решается? Просто всё льём в одну ветку?
                                        0
                                        Да
                      +6

                      Полностью согласен с коллегой edogs выше, но от себя тоже хочу добавить и более жестко.


                      Совет "задавайте открытые вопросы" — это идиотский совет. Люди, которые это советуют, занимались ерундой во время всех тех тысяч код-ревью, которыми они хвастаются.


                      Я расскажу вам, какой будет результат от открытого вопроса.


                      1. Наиболее вероятный результат — вы получите ответ "Вроде всё нормально, проблем не вижу". Дальше, если вы продолжите гнуть свою линию и зададите еще один открытый вопрос в духе "Точно нормально? Уверен?", то получите ответ вроде "Да, уверен! Чо ты дебильные вопросы задаёшь?! Если знаешь что надо исправить — скажи нормально, не выноси мозг!". Ну и дальше скандал, ругань, трата времени и испорченные отношения.


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



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

                        +3
                        >Люди, которые это советуют, занимались ерундой во время всех тех тысяч код-ревью, которыми они хвастаются.
                        Да, кстати, 1000 ревью за три года — это всего лишь примерно одна штука в день. Я вообще не вижу, чем тут хвастаться, у меня постоянный поток код-ревью от коллег примерно такой. Автор уже три года как стал миддлом, и начал делать код ревью? Ах, какое достижение…

                        >Нельзя никогда задавать открытые вопросы на код-ревью.
                        Ну, в какой-то мере код-ревью — это все-таки средство для обучения менее опытных более опытными коллегами. А в обучении разные подходы могут быть хороши, смотря кто перед вами. Но лишь могут…

                        Поэтому мне кажется, тут вообще нет одного правильного ответа. И совет «всегда задавайте открытые вопросы», и совет обратный — они оба не вполне универсальные.
                        +4
                        Добавьте флаг «Я запускал этот код локально»

                        как раз лишний пункт,
                        то что компилируtтся, запускается и работает — должно проверятся тестами в build pipeleine
                          –1

                          А все равно бывает, что CI driven разработка дает сбой

                            0

                            Какое у вас тестовое покрытие? Как оно измерено?

                              0
                              конечно не 100%
                              но ошибки «не компилируется, не запускается» — ловит
                              бизнес логика достаточно сложная — руками просто не проверить ВСЕ вариации с разными входными данными, настройками

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