За последние 3 года я рассмотрел более 1000 pull (merge) request’ов. За это время я многому научился — в основном тому, как не проверять код, как сделать процесс менее болезненным, что делает код хорошего качества и так далее.
Это самая важная вещь, на которую стоит обратить внимание.
Делая code review, вы должны держать в голове много вещей. «Что за этим стоит?», «Как это согласуется с остальной частью кода?» и «Будет ли это хорошо работать?» Вот лишь некоторые из вопросов, на которые нужно ответить. Таким образом, когда у вас есть pull request, который пытается решить одну проблему, на некоторые из этих вопросов легче ответить.
Другим важным аспектом является размер pull request’а. Большие запросы требуют экспоненциально больше времени для рассмотрения. И когда я узнаю, что мне нужно потратить более 15 минут на запрос, вам придется подождать до пары часов.
Более крупные запросы также содержат больше ошибок, поэтому время на утверждение также значительно возрастет. Это означает, что у вас может быть код, ожидающий подтверждения в течение нескольких дней. И если ваша компания гибкая, это увеличивает шансы на конфликты, которые очень болезненны.
Так что следите за частыми ловушками, такими как переименования, обобщения кода и тому подобное. Хотя он невиновен и сделан с добрыми намерениями, он убирает внимание с важных частей — повышать качество кода и уменьшать количество ошибок. Просто сделайте свою гениальную идею в другом запросе.
Продолжая мысль о том, что рецензент должен держать в голове много вещей, включая проверку форматирования кода, наличие соответствующей документации, проходимые тесты и так далее. Я помню время, когда мне приходилось принимать все это во внимание — это отвлекало и отнимало много времени.
Хорошо, что есть довольно простое решение — интегрируйте все проверки в CI. Тогда у вас будут запускаться все проверки всякий раз, когда кто-то отправляет pull request, и не разрешает слияние пока все проверки не пройдут успешно. Вам как рецензенту больше никогда не придется беспокоиться о форматировании.
Автоматические тесты помогают убедиться, что автор ничего не сломал, и что тесты все еще проходят. В зависимости от вашего подхода к тестам, это, пожалуй, самая часть в работе вашей CI.
Автоматическое форматирование кода приводит к тому, что все споры вокруг идеальной длины строки или места добавления новой строки исчезают. Просто решите командой какой набор правил форматирования вам подходит и передайте его автоформатору. Это избавит вас от многих неприятностей. Если у вас возникли проблемы с согласованием формата, посмотрите, как это делают Google, Facebook или другие крупные компании.
Автоматический линтер также очень важен для таких языков, как Python, где форматирование кода влияет на логику кода. Большинство форматеров кода для Python форматируют только тот код, который, как они знают, точно не повлияет на логику. Добавив линтер, вы решите все скрытые проблемы.
Также заслуживают упоминания проверки типов и документации, но самое главное — автоматизировать проверки, которые имеют смысл для вас и вашей команды. Если вы думаете, что что-то поможет, обсудите это и попробуйте в течение недели или двух.
Это ускоряет процесс рецензирования, поскольку вы можете быстрее перебирать код с автором и делиться своей точкой зрения. Автор также может лучше объяснить причину своего подхода, например, если он уже что-то пробовал и почему это не сработало.
Это также может быть отличной возможностью больше общаться с людьми, что очень важно. Когда ваши коллеги-разработчики увидят, что вы инвестировали в их личный и деловой рост, они это оценят. Для вас и автора это также прекрасная возможность попрактиковаться в общении, что крайне важно для хорошо функционирующих команд.
Будьте осторожны, чтобы не переусердствовать, хотя. Некоторые pull request’ы слишком малы и слишком просты, чтобы вести какой-либо эффектный разговор. В этом случае встреча с автором может быть пустой тратой времени.
Если вы более опытны, чем автор кода, вы должны принять во внимание, что то, как вы говорите, имеет большое значение. Хорошо написанная критика может помочь разработчику стать лучше в будущем, но она также может разрушить чьи-то мечты.
Я обнаружил, что лучше всего задавать открытые вопросы — это не агрессивно и даже побуждает разработчика мыслить критически. Это займет больше времени, чем просто рассказать кому-нибудь решение? Да, краткосрочно. Но в долгосрочной перспективе вы помогаете им расти, и они с меньшей вероятностью повторят свои ошибки.
Итак, в следующий раз, когда кто-то откроет файл внутри цикла for, а не перед ним, вместо того, чтобы просто указать на это, спросите: «Как бы вы уменьшили сложность здесь?». Это будет много значить.
Больше всего меня беспокоит обнаружение ошибки в небольшом запросе, из-за которого функциональность вообще не работает. Это означает, что разработчик даже не запустил код — вероятно, думая, что в этом нет необходимости, поскольку изменения настолько малы.
После того, как это случилось пару раз, я добавил флаг под названием «Я запускал этот код локально», что полностью решило проблему. Я прекратил просмотр кода, который не запускался локально, и люди не лгали об этом. Не было обид, потому что мы все знали, что это нужно делать.
Бонус: это наш шаблон, который должен заполнить каждый разработчик при создании pull request’a:
Узнайте подробности, как получить востребованную профессию с нуля или Level Up по навыкам и зарплате, пройдя платные онлайн-курсы SkillFactory:
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
Должен знать о
Есть что-нибудь еще, что должно быть известно?
Любые замечания по развертыванию?
Любая дополнительная документация?
Узнайте подробности, как получить востребованную профессию с нуля или Level Up по навыкам и зарплате, пройдя платные онлайн-курсы SkillFactory:
- Курс по Machine Learning (12 недель)
- Курс «Профессия Data Scientist» (24 месяца)
- Курс «Профессия Data Analyst» (18 месяцев)
- Курс «Python для веб-разработки» (9 месяцев)
edogs
Я обнаружил, что лучше всего задавать открытые вопросы — это не агрессивно и даже побуждает разработчика мыслить критически. Это займет больше времени, чем просто рассказать кому-нибудь решение? Да, краткосрочно. Но в долгосрочной перспективе вы помогаете им расти, и они с меньшей вероятностью повторят свои ошибки.
Итак, в следующий раз, когда кто-то откроет файл внутри цикла for, а не перед ним, вместо того, чтобы просто указать на это, спросите: «Как бы вы уменьшили сложность здесь?». Это будет много значить.
laphroaig
Сдается мне, что вся эта «западная толерантность» не на пустом месте. Не знаю с чем связанно, но напрямую указать на проблему без открытой агрессии и экспрессии они не могут. Т.е. у них нет опции просто сказать «пива нет», а только либо пафосно «Пива-а не-е-е-т!!! » либо толерантно поставить открытый вопрос
iproger
Я думаю вы правы. Естественно за всех говорить нельзя. Но вот улыбки так же не на пустом месте взялись. Видимо, вынужденная мера чтобы все не перегрызлись.
ApeCoder
Это у всех так. Попробуйте внимательно понаблюдать за интонациями коллег при обсуждении каких-либо разногласий. Почти каждому нужно эмоционально самовыразиться.
Просто у нас это настолько повсеместно, что считается нормальным.
sumej
Мальчику стажеру я когда-то на пул реквестор в 20 строк написал много замечаний и рекомендаций прикрепив примеры и вышел за рамки экрана, а так же использовал такое:
Так не правильно, потому-то
Это не сработает, так как
Это сложно/Зачем это
Найди другой способ решения согласующийся с
Человек испугался :(
Вообще не важно человек писал код 5 минут или час или скопировал где. Как только пишешь, что мол код негодный и вот места с вопросами — мотивация у него падает и ещё агресия встречается.
Люди сами не могут код от себя отделить.
Люди не могут просто так отказаться от своей идеи.
Люди не хотят тратить время на переделывание, «так как делают по максимуму», а это «больше придирки и вкусовщина» и даже жалуются на выгорание и отсутствие мотивации.
Часто получается так, что они хотят ПР пройти, а не код написать.
edogs
И что они делают в программерах? В профессии, когда обучение никогда не заканчивается по определению?
sumej
Мой опыт: Я с русскоговорящими в том числе работаю. И казалось бы такая «закалка» и «конструктивная» критика, которая даётся в СНГ должна давать преимущество.
Но я все время сталкиваюсь с такой проблемой, что люди не готовы озвучивать свои мысли.
«Я это сделаю. Я все понял» через неделю заканчивается тонной велосипедного кода.
А в начале же говоришь, что можно созвониться или початиться или мыло отправить, если есть вопросы или недопонимание. Но нет.
А помощь нужна? Конечно же нет.
У «толерантных» коллег меньше проблем и созвониться и прогресс показать, и закончить велосипедить, начав код писать.
Ну это по моим личным наблюдениям.
DaneSoul
Конечно подход не универсальный и не для всех задач, но те решения до которых додумался самостоятельно запоминаются намного прочней.
BekzhanKassenov
Думаю все-таки очень важно следить за лексиконом во время код-ревью. Даже такие слова как «неадекватно» могут выглядеть чересчур резкими. Помните, что вы и без этого в позиции силы, аппрувал кода зависит от вас и нет нужды дополнительно демонстрировать эту силу.
Я бы переформулировал эту фразу более нейтрально: «Этот файл открывается в цикле, это может сильно сказаться на сложности/производительности кода». Желательно дополнить это еще советом (но не указанием, автор может придумать что-то лучше чем вы) о том, как этот код можно улучшить. Например: «Попробуйте переписать код так, чтобы открыть файл один раз перед циклом и переиспользовать дескриптор»
edogs
Во-первых, термин «неадекватно» (применительно к коду/решению) это всего лишь «не соответствующий чему-либо, неподходящий для чего-либо». Таким образом когда Вы называете это слово слишком резким — Вы ставите субъективное мнение выше объективного значения и тем самым открываете врата ада. Когда кодер должен гадать не только о том, где он сделал что-то не так, но и еще гадать как бы ему ответить так, что бы это не звучало для ревьюера слишком резко с точки зрения субъективного мнения ревьюера. Подобная атмосфера не способствует взаимопониманию и продуктивности.
Во-вторых, использование Вами фразы «с позиции силы» предполагает что в коллективе нет здоровых, партнерских отношений и взаимопомощи, а есть отношения подавления и силы и hostile environment (в голову не приходит как сказать это по русски, извините). И проблема именно в этом, использование терминологии может маскировать эту проблему, но никак ее не решит. В коллективе должна быть такая атмосфера, что бы наличие позиции силы у вышестоящего не подразумевалось, тогда любые замечания будут восприниматься в позитивном ключе.
Как бы вопрос не формулировался — первое что должен сделать ревьюер — узнать почему код написан так, как он написан. Выяснив причину — может оказаться что код на самом деле написан верно. И даже если не верно — выяснение причин поможет избежать аналогичных ошибок в будущем, а не просто поможет устранить эту конкретную.
BekzhanKassenov
Тем не менее слово «неадекватно» гораздо чаще применяется в виде оскорбления и применение его в работе по отношению к коду созданному другим человеком может уменьшить желание следовать этому комментарию.
Полностью с вами согласен, субъективность восприятия людей все меняет. Возможно мне не стоило поднимать эту бессмысленную дискуссию о степени резкости одного слова, но мой глаз за него зацепился в изначальном комментарии.
Быть в позиции силы не означает, что ее нужно использовать, но все же у ревьюера она есть, и это факт, о котором стоит помнить.
Не могу согласиться со словом «любые», опять-таки это слишком субъективно и нужно помнить что разные люди воспринимают жесткую критику по-разному.
P.S.
Чисто из интереса: считаете ли фразу, которые я предложил как коменты в код-ревью излишне толерантными? Такой же вопрос к общему стилю моих комментариев в этом треде.
edogs
Возможность «пожаловаться» должна быть абсолютно штатной и должна уравновешивать «силы». И это даже вопрос не толератности, а продуктивности.
Воспринимают по разному. Но если кто-то воспринимает замечания к работе слишком остро, то проблема на его стороне. Именно потому, что это слишком субъективно и именно воспринимиющий отвечает за свое восприятие.
Излишне толерантными — нет, вот вообще ни разу, если отвечать строго на этот вопрос.
pankraty
Ну нельзя же настолько откровенный гуглоперевод публиковать!
sim2q
пока думал как бы об этом в стиле статьи вокруг да около написать — опередили:)
Keyten
skillfactory_school, как бы вы улучшили качество статьи?
Yoooriii
Вставлю свои пару центов:
sshikov
>Pull request должен делать только одну вещь
Тут проблема в том, что этот совет неконструктивен. Чтобы его выполнить, нужно определить, что такое эта «одна вещь». Собственно, вы это почти и иллюстрировали своим комментом.
Кроме того, разбить PR на несколько мелких — значит получить в N раз больше PR, которые непонятно как связаны друг с другом (потому что проследить эту связь между разными PR весьма проблематично). Такое лечение вполне может оказаться хуже болезни.
>Добавьте флаг «Я запускал этот код локально»
Во многих моих проектах код просто локально не запускается. Например потому, что работает в среде Hadoop — а локальные имитации кластера это просто убожество. Я понимаю, почему эту идею поддерживают, но она далеко не универсальна.
ApeCoder
Почти все идеи неуниверсальны. Это не значит, что их не надо поддерживать. Надо просто применять там, где это пододит.
Наверное здесь "локально" означает "в тестовой среде" она может быть нелокально, а в облаке, просто код в тестовом инстансе развернут из данного конкретного бранча. Судя по тексту, автор имел ввиду "я провел ручное End-To-End тестирование этого кода"
sshikov
Ну, проблема в том, что авторы большинства таких статей не понимают (или что еще хуже — скрывают) факт неуниверсальности своих советов. Поэтому советы по большей части получаются из серии «лучше быть богатым и здоровым».
panchmp
автор данной статьи просто написал что то в «свой бложик» для новичков
а переводчику вообще пофиг что переводить, лишь бы продвигать свои курсы
sshikov
Согласен
sved
Я для себя давно пришёл к выводу, что ветки надо создавать не по фичам, а по разработчикам. Это прям снимает кучу проблем.
sshikov
А одну фичу при этом делает сколько людей?
sved
Смотря что вы подразумеваете под фичей. Если таску в джире, то её делает один человек. И один пул реквест покрывает потенциально несколько тасок.
В общем читать здесь: www.atlassian.com/git/tutorials/comparing-workflows/forking-workflow
sshikov
Не очень понятно, какую именно кучу проблем этот подход снимает, и почему не создает другие.
Ну ок, один человек ведет свою работу в своем репозитории. И делает несколько задач.
Понятно что у него-то все будет хорошо — пока не нужно будет эти таски объединять с чужими в рамках одного релиза. Но в этот-то момент что помешает возникновению конфликтов, например?
sved
Потому что мы мержим конкретный код у которого есть конкретный автор, а не просто кучу абстрактных бренчей, которые мы создаём тысячами.
Автор всегда имеет удалённую копию и может синхронизировать свою работу.
Всегда можно посмотреть кто над чем работает.
Работа разработчика не блокируется, если пул реквест отсрочен. Он может продолжать мерджить из мастера и продолжать свою работу.
И такси надо объединять не в рамках релиза, а постоянно.
ApeCoder
Вы имеете ввиду что на одного разработчика ровно одна ветка? Или что ветка на фичу И разработчика?
Ссылка на forking workflow она про создание форков репы а не про ветки.
Если разработчик заводит ветку на инкремент (багу или новую фичу) то все плюсы, которые у вас упомянуты уже соблюдаются.
Разрабочик создает ветку, ассоциируюет ее с таксой в багтрекере — сразу видно кто занимается, после того как он закончил работу он делает пулл реквест и изменение попадает в мастер после ревью. В процессе работы он может мерджить свежий мастер не блокируется другими людьми.
sved
Один разработчик — одна ветка. Заводить по ветке на фичу — крайне неудачная идея по моему опыту.
Во первых возникает туева хуча веток. Часто работу нельзя правильно разбить по этим веткам, да и не нужно.
Если ревьювер занят, то накапливается куча веток которых надо смержить друг в друга а также в локальную ветку. Это крайне неудобно.
Кроме того разработчики часто работают над несколькими задачами одновременно. Надо постоянно переключаться между ветками.
Непонятно куда девать коммиты которые не связаны непосредственно с тасками, либо таски не созданы, либо уже закрыты.
Forking workflow же вообще не имеет никаких недостатков и выглядит более элегантно.
ApeCoder
Что такое "правильно разбивать" и почему их невозможно разбить?
Что их заставляет так делать? Как они поступают когда одна задача готова, а другая — еще нет? Как потом в истории разобраться зачем именно было сделано конкретное изменение?
Давайте, переведу на русский — непонятно, куда девать комиты которые не связаны с задачами, либо задач нет либо они уже выполнены.
С моей точки зрения, все коммиты связаны с задачами, если у вас нет никакой задачи не создавайте коммит. Если у вас есть задача, оформите это в багтрекере. Если у вас коммит связан с закрытой задачей, значит в багтрекере вранье — либо задача не закрыта, либо это новая задача, связанная с данной.
sved
Что значит зачем? Программирование — это творческий процесс. Разработчики работают, как им удобнее. Если у меня есть 2 задачи по фронту и бакенду для фичи A и фронт+бакэнд для фичи B.
То мне может показаться удобнее написать сначала бакэнд для обеих фич а затем фронт для обеиз фич.
В коммитах должно быть описание. Ваши ветки в этом, кстати, мало помогают. Ибо разработчики смотрят историю основной ветки (как правило).
То что вы описываете-это самая настоящая бюрократия. И она плохо ложится на реалии.
Я например постоянно переписываю документацию. И что мне для каждой опечатки таску и отдельную ветку создавать?
ApeCoder
Ок тут согласен — вполне допустимо в каких-то случаях создавать ветку на несколько связанных задач, если их удобнее сделать как одну.
Но вы-то предлагает ветку на разработчика — т.е. там должны быть вообще все зачачи разработчика даже логически не связанные или как?
Наверное тут какое-то недопонимание. Я подразумеваю, что ветки сливаются в какой-то момент в основную. Соответсвтенно, merge commit будет содержать описание и ссылку на задачу и можно будет посмотреть в истории по master.
История в фичебранчах как правило не нужна другим разработчикам, кроме того, случая, когда они совместно работают над долгоживущим бранчем. Но этот случай вообще непонятно, как решать в режиме "ветка на разработчика".
Ага, с моей точки зрения это не должно занимать сильно больше времени, чем написание данного коммента.
sved
Совершенно верно. Собственно так это и задумывалось: youtu.be/4XpnKHJAok8?t=1599
Merge commit будет содержать название ветки, но что мне это даст? Не проще ли просто посмотреть в комментарии к коммитам.
Как фича бренчи улучшают понимание, если разработчики синхронизируются только с мастер веткой? Они могут появляться и исчезать без всяких следов. Так зачем их создавать тогда?
Я не спорю что есть ситуации, когда надо действительно сделать какую-то отдельную изолированную ветку длиной в полгода. Но зачем засорять гит тысячами мелких бренчей мне непонятно.
Это сильно раздражает. Это всё равно что раз в час писать в журнал что сделал за последний час — недолго, но мерзко
ApeCoder
Приведите цитату плиз, я там услышал что им удобно репо на команду, а не ветку на разраба
Обычно он содержит ссылку на PR которую можно открыть и посмотреть все детали
Если они синхронизируются с мастером, то, по крайней мере, можно для работе в процессе понять какие изменения для какой фичи предназначены. Можно так же мерджить их в мастер пофично (всегда решить какую фичу вмерджить, а какую — нет). Для готовой работы есть уже мастер с его историей.
Так они его не засоряют. Обычно они живут пока фича разрабатывается и умирают после мерджа в мастер — причем, это автоматически происходит при завершении пулл реквеста. При следовании соглашениям о наименовании для бранчей типа feature/AddLoginDialog их можно не видеть, пока явно не захочешь, но зато, если захочешь можно увидеть.
sved
Почему нельзя взглянуть сразу в коммит сообщения, где так же будет ссылка на таску в джире (если коммит связан с таской). Зачем мне идти в мердж коммит, который фиг знает где, открывать пул реквест и только там смотреть конкретные таски?
Зачем что-то мержить в местер пофично? Если какая-то фича не вмержена в мастер, то я как разработчик фичи нахожусь в подвешенном состоянии ибо не знаю, какую версию кода брать для дальнейшей работы над другими задачами: с фичей или без неё.
ApeCoder
Что значит фиг знает где? Он составляет историю мастера.
Я совершенно не против, чтобы там были и идентификаторы фич. Просто если их много удобнее видеть ссылку PR в целом и текстовый осмысленный коммент, если нужны детали можно открыть PR и ходить по ссылкам на фичи если их несколько.
Если она одна, наверное, иметь ссылку на PR чуть менее удобно, чем на фичу в джире на один дополнительный клик. Зато имея ссылку на PR можно посмотреть и дополнительные вещи (типа кто ревьюил и какие были замечания.)
У меня обычно наиболее гранулярные коммиты содержат атомарные правки типа "переименование метода", который мало кому интересны кроме меня, но я зато всегда могу откатиться на предыдущую версию в процессе работы над фичей, но там обычно нет ссылки на багу. На багу ссылается ветка в целом.
Потому, что вы можете работать, как вы сами говорили, над несколькими фичами, одна может быть готова отправке в мастер, другая не готова — тесты не дописаны, не компилируется, нет документации или какие там у вас характеристики проверяет билд еще.
Если она не вмерджена в мастер, значит не готова. Берите всегда из мастера. (Кроме того случая, когда вы работаете над долгоиграющей фичей — тогда берите из фичи, над которой работаете)
sved
Вы пытаетесь использовать ветки для документации истории изменений. Но они для этого не предназначены. Они нужны для изоляции кода.
Для документации изменений существуют комментарии к коммитам, где может быть любая исчерпывающая информация. Мердж коммит находится «вдалеке» от основного коммита и надо искать где находится тот, про который написано в мердже.
А в мастер не обязательно мержить, когда вся фича готова. Можно замерджить и 10 фич и полфичи. Главное — чтобы существующий код не ломался и был проревьювен.
Необязательно: она может быть сделана, но непроревьювена.
Rukis
А как быть если на ревью развернули одну фичу из пакета? Получается все фичи, в том числе не связанные, встают из-за одной. И если вы не придерживаетесь принципа «не сделано — пока не прошло ревью», то вы можете отнаследоваться от кода, который потребует глубокой переработки и сделать много лишней работы.
ApeCoder
Как раз для изоляции — фич друг от друга.
Коммиты бывают разные — например в процессе разработки фичи могут быть эксперименты, откаты на предыдущие версии, мердж с мастером и так далее.
Все это нужно до того момента, когда надо интегрировать фичу, после того как фича готова, это можно засквешить.
Делается squash merge и я вижу только один коммит. Если не хочется сквешить, то можно проребейзить перед мерджем и они будут рядом.
Чем больше объем мерджа тем больше разбираться что сломало CI, меньше гранулярность при черрипике, если надо будет, больше объем ревью и так далее.
Полфичи можно, но тут надо понять, чтобы не было неконситентного нового наблюдаемого поведения. Если его нет, то либо эти пол фичи можно выделить в фичу, либо будет мертвый код. Хотя, это, в принципе допустимо, многие так делают изолировав новое поведение фичетаглами.
Зачем ее тогда брать, после ревью может быть сильно изменена?
Я бы сказал, что это нетипичный случай.
sved
Ну правильно. Но зачем изолировать задачи выполняющиеся одним и тем же автором, на протяжении короткого времени и часто вперемешку. А после мерджа в мастер всё равно обе модели выглядят по сути одинаково.
Даже если допустить что мерж коммиты дают кое-какую дополнительную информацию, то на практике я не припомню, чтобы я когда-либо с этим заморачивался. Обычно я смотрю на конкретные коммиты.
Ну вообще это больше зависит от пропускной способности ревьюверов, а не от бренчинг модели.
Вообще, конечно, чем чаще мерджинг тем лучше, хоть полфичи хоть четверть. Feature branch модель скорее неявно поощряет откладывать мерджинг, до тех пор пока таска не полностью завершена
dopusteam
А в случае с одной веткой на разраба как эта проблема решается? Просто всё льём в одну ветку?
sved
Да
ivanych
Полностью согласен с коллегой edogs выше, но от себя тоже хочу добавить и более жестко.
Совет "задавайте открытые вопросы" — это идиотский совет. Люди, которые это советуют, занимались ерундой во время всех тех тысяч код-ревью, которыми они хвастаются.
Я расскажу вам, какой будет результат от открытого вопроса.
Наиболее вероятный результат — вы получите ответ "Вроде всё нормально, проблем не вижу". Дальше, если вы продолжите гнуть свою линию и зададите еще один открытый вопрос в духе "Точно нормально? Уверен?", то получите ответ вроде "Да, уверен! Чо ты дебильные вопросы задаёшь?! Если знаешь что надо исправить — скажи нормально, не выноси мозг!". Ну и дальше скандал, ругань, трата времени и испорченные отношения.
Второй по вероятности результат, если автор кода мямля, это сделанные наобум бессмысленные правки в попытке угадать что вы там имели в виду. Потом ещё и ещё раз, потом уже вы сами придёте в ярость и обругаете автора кода, что он идиот и не понимает очевидного для вас. Опять же скандал, ругань, трата времени и испорченные отношения.
Нельзя никогда задавать открытые вопросы на код-ревью. Это не тот этап в цикле разработки, где нужно проявлять фантазию, остроумие и творческие таланты. Если видите проблемы — говорите прямо, а не ходите вокруг да около, не тратьте время на ерунду.
sshikov
>Люди, которые это советуют, занимались ерундой во время всех тех тысяч код-ревью, которыми они хвастаются.
Да, кстати, 1000 ревью за три года — это всего лишь примерно одна штука в день. Я вообще не вижу, чем тут хвастаться, у меня постоянный поток код-ревью от коллег примерно такой. Автор уже три года как стал миддлом, и начал делать код ревью? Ах, какое достижение…
>Нельзя никогда задавать открытые вопросы на код-ревью.
Ну, в какой-то мере код-ревью — это все-таки средство для обучения менее опытных более опытными коллегами. А в обучении разные подходы могут быть хороши, смотря кто перед вами. Но лишь могут…
Поэтому мне кажется, тут вообще нет одного правильного ответа. И совет «всегда задавайте открытые вопросы», и совет обратный — они оба не вполне универсальные.
panchmp
как раз лишний пункт,
то что компилируtтся, запускается и работает — должно проверятся тестами в build pipeleine
Gorthauer87
А все равно бывает, что CI driven разработка дает сбой
ApeCoder
Какое у вас тестовое покрытие? Как оно измерено?
panchmp
конечно не 100%
но ошибки «не компилируется, не запускается» — ловит
бизнес логика достаточно сложная — руками просто не проверить ВСЕ вариации с разными входными данными, настройками