Comments 20
А как быть, если рефакторинг требуется для решения задачи?
Сначала сделать ветку с рефакторингом и пока он апрувится от этой же ветки делать фичу?
Просто вижу проблемы с асинхронностью: обычно необходимость рефакторинга осознается, когда работаешь с кодом в рамках какой-то задачи. Как тут встроить рефакторинг в процесс, чтобы не тормозить основную задачу?
Навскидку вижу два подхода
Порефакторить после фичи. Фича первична, рефакторинг обычно не критичен.
Порефакторить внутри задачи. Ревью будет дольше, как автор в статье указал, но это будет целостное (и необходимое?) изменение по задаче.
По моему личному опыту проще делать второе. По тому что предлагает автор в статье - я бы сказал что первый вариант тоже может быть неплох. Но опыта регулярных мелких рефакторингов у меня нет, сложно судить.
Как человек написатель/читатель ПРов вижу такое противоречие:
Программист хочет рефакторить вместе с задачей, потому что он видит актуальные проблемы кода, ему не удобно и т.п.
Ревьюер хочет смотреть рефакторинги отдельно от задачи, чтобы уменьшить сложность и повысить качество ревью.
Возможное решение: пока делаешь основную задачу, кидаешь ПРы с микрофакторингами в отдельных ветках. Эти ветки можно подтягивать в свою ветку как до и так после прохождения ревью.
Это пока они друг друга не цепляют, эти изменения. Когда рефакторинг и фича пересекаются, так уже не сделать. Я собственно так и понял было изначальный вопрос.
Обычно в ревью можно посмотреть изменения по отдельным коммитам и если явно указать, что коммит А - вспомогательный рефакторинг, а Б - фикс бага, то и в одном ревью удобно.
Ещё важно как изменения доезжают в мастер - могут ребейзиться все (тогда в истории коммит с рефакторингом будет отдельно от изменений и всё ок), а могут сквошиться в один (и тогда может быть есть смысл разделять рефакторинг и исправления на отдельные ПР).
Если PR-ов с микрочистками становится много, они начинают тормозить процесс, и мы попадаем в "ловушку 2". Время работы над задачей всё равно растягивается, да и ревьюверы всё равно ворчат, что у них много работы. И вопрос сцепления изменений по-прежнему остаётся острым. Поэтому я всё же предлагаю идеи по чисткам не прокатывать сразу, а сперва закидывать в беклог, "грязный" же код ещё какое-то время терпеть. Если/когда конвейер асинхронных чисток налаживается, то самые неприятные куски кода быстро будут вычищены.
Если рефакторинг прям действительно необходим для текущей задачи, варианта вижу два:
Да, отдельная ветка с предварительным рефакторингом и отдельный PR. Пока аппрувится, работать над самим изменением. Абсолютно нормальный и хороший вариант, хотя порой и приходится несколько больше обычного возиться с
gitлокально.А порой и всё вместе катить тоже норм, если изменения в целом достаточно небольшие.
В асинхронные чистки я стараюсь убирать те изменения, которые мозолят глаз, но не критичны для текущей работы.
Пример: представим, что команда поменяла недавно в проекте базовый уровень языка. Стали доступны новые плюшки синтаксиса, в стандартной библиотеке появились новые функции, которые раньше были только в сторонних зависимостях или собственных утилитах. А какие-то конструкции кода теперь признаны устаревшими.
Стоило бы обновить код на проекте в целом? Скорее всего, да - это улучшило бы читаемость и понятность, позволило бы упростить код, позволило бы (к примеру) убрать какие-то зависимости, ставшие необязательными. Стоит ли делать это в рамках обычной работы? Скорее всего, нет. Это будет раздувать PR-ы и несколько увеличивать риск поломки. Да и не везде получится поменять стиль - только в "горячих" участках кода. А в "холодных" будет всё по-старому, и стиль на проекте в среднем станет хуже (мешанина старого и нового, выше энтропия).
Зато для асинхронных чисток это подходящая цель. В каждом отдельном PR берём и перерабатываем ровно одну конструкцию со старого стиля на новый. Равномерно по всему коду. Повсюду распространяется модный и красивый новый стиль, но при этом мы не сильно много тратим времени на эти изменения.
Чтобы не тормозить текущую задачу, используем любой вид беклога. Записываем идею по чистке вообще в любое место, откуда мы уверены, что её потом прочитаем и не потеряем. Что это за беклог - каждый решает для себя сам. Кому что удобнее: бумажка, файл на компе, голосовуха самому себе в телеге, и т.п. Потом, когда голова не загружена текущей задачей, проверяем свой временный беклог и переносим идею из временного места в официальный беклог для таких чисток. Получается примерно как в GTD или "Джедайских техниках" Дорофеева.
Видел вполне нормальные команды и тимлидов, в которых в спринт включается не только набор текущих тасок, но и под каждую выделяется примерно 10% её времени на бэклог и рефакторинг. Вплоть до того, что при раздаче спринта к каждой таске приклеиваются задачи из бэклога.
В целом неплохая статья со сборкой достаточно актуальных вопросов. Спасибо.
На это нужна политическая воля
Хорошо, конечно, когда есть гарантированные 10% на бэклог и рефакторинг. Но такая радость встречается не всегда. Предложенный метод позволяет, среди прочего, работать над качеством кода "ниже радара", чтобы, с одной стороны, не терять фокусировки на текущие задачи (даже если в них никаких чисток не запланировано), а с другой стороны, постепенно снижать боль, которая возникает от "грязного" кода. Я не призываю всегда чистить код "ниже радара", но порой других вариантов не остаётся. Потом, уже при наличии успешной истории чисток, проведённых без ущерба для работы, можно и поднять вопрос о легализации подхода, например.
Вплоть до того, что при раздаче спринта к каждой таске приклеиваются задачи из бэклога
А этот подход для меня, скорее, антипаттерн (ловушка 2 из вступления). По крайней мере, в том случае, когда изменения по разным задачам смешиваются в одном PR. Если они идут разными патчами/реквестами, то вполне норм.
Спасибо за отличную статью! Теперь к привычной цепочке добавилась ещё одна continuous. Однако для меня остаётся открытым вопрос, что делать с комитами? При таком подходе история дико раздуется и всю эту красоту в мастер сливать, мягко говоря - не правильно.
В чём проблема?
Я лично ничего не делаю с историей, получилась фича из 20 коммитов - пусть так и будет.
Лично мне обычно бывает проще разобраться в сотне маленьких коммитов, чем в десятке больших, получившихся из их объединения. Другой вопрос, что этот вопрос всё равно должен решаться на уровне каждой отдельной команды и/или проекта индивидуально, исходя из опыта сотрудников, текущих процессов, инфраструктуры, требований к сборке и т.п. Но это касается обычных задач.
Если же мы говорим про регулярные подчистки, тут я всё же за подход "1 чистка = 1 небольшой коммит = 1 мердж в мастер"). Посмотрите ещё раз "Принцип 6. Асинхронность". Я стараюсь как можно быстрее вливать чистки в master, чтобы они распространились как можно быстрее по всему коду, включая код коллег.
Спасибо за статью и за Mikado Method.
Не очень понятно в чём обязательность "1 небольшой коммит = 1 мердж в мастер". Я работал в команде, где было принято делать маленькие коммиты, выкладывать на код ревью ветку состоящую из множества маленьких коммитов и, собственно, делать ревью по коммитам. Это требует некоторой дисциплины и умения пользоваться `git rebase --interactive`, поскольку процесс написания кода не линейный и изменения идут не в том порядке и не в той гранулярности чтобы было удобно потом ревьюить. При помощи `git rebase --interactive` коммиты переупорядочиваются, сливаются или, наоборот, разбиваются чтобы потом выложить на ревью изменение в таком виде что рефакторинги (вернее, префакторинги) выделенны в отдельные коммиты, но всё же включены в общее изменение. Так вот, преимущество в том, что нет необходимости отвязывать рефакторинг от работы над фичами и заставлять себя рефакторить то, что никто сейчас не трогает.
Более того, можно вооружиться идеей "работает - не трогай" и спокойно включать пререфакторинг в основную работу. См https://ronjeffries.com/xprog/articles/refactoring-not-on-the-backlog/
Ещё в качестве легкого бэклога рефакторинга работают `// TODO: ...` комментарии прямо в коде. Есть TODO - есть что фиксить. Пофиксили - нет больше TODO. Нет необходимости расписывать контекст в тикете или давать ссылки, которые устаревают, а также нет проблемы рассинхронизации тикета и кода. Естественно, это работает когда репозитариев немного.
Порадовала ссылка на работу Драгана. Я работал с ним немного в одной организации, и благодаря статьям, которыми он регулярно делился, я узнал что есть разумная жизнь в виде agile development практик.
Не очень понятно в чём обязательность "1 небольшой коммит = 1 мердж в мастер". Я работал в команде, где было принято делать маленькие коммиты, выкладывать на код ревью ветку состоящую из множества маленьких коммитов и, собственно, делать ревью по коммитам
Мне кажется, Вы сами ответили на свой вопрос. В команде, где такая практика не выстроена, ревью проводить гораздо тяжелее. Пришёл на ревью PR с одним коммитом, где все типы изменений перемешаны, - и что с ним делать? Я лично сам обычно тоже стараюсь сортировать и упорядочивать свои коммиты, но не уверен, читает ли их кто-либо в подготовленном порядке.
Кроме того, асинхронный подход "1 небольшой коммит = 1 мердж" уменьшает риски возникновения конфликтов при слиянии изменений от разных разработчиков. Чем дольше PR висит в ревью и чем больше файлов он зацепляет, тем у нас в среднем выше риск появления конфликтов между разными ветками. Расчёт у меня такой: большой PR ревьювится медленно, поэтому постараемся не проводить в нём опциональные чистки, чтобы (среди прочего) уменьшить объём и тем самым снизить риск пересечения с другими ветками. А в асинхронном PR с чистками, наоборот, сделаем одно небольшое и простое изменение, но зато в большом количестве файлов. Стараемся, чтобы это изменение быстро прошло ревью и стало доступно всем - а значит, увеличиваем шансы на то, что следующая ветка, которая стартанёт от master, уже будет в себя включать это изменение. Так мы уменьшаем риск появления конфликтов.
Если эту технику довести до экстремума, то мы получим Trunk Based Development, где все изменения попадают в главную ветку максимально быстро. На моей практике, далеко не все команды и программисты бывают готовы к такому подходу. Предлагаемый механизм "асинхронных чисток" немного приближает флоу к TBD, не нарушая при этом церемоний, связанных с ревью PR-ов.
Ещё в качестве легкого бэклога рефакторинга работают
// TODO: ...комментарии прямо в коде.
Я рад, если у Вас они работают. Мой опыт работы с тудушками несколько более плачевен. Скорее всего, им нельзя давать разрастаться в количестве. Потому что когда это происходит (или, когда Вы приходите на легаси-проект), сама процедура выбора "следующей тудушки на фикс" начинает занимать слишком много времени.
В том и дело, что я их не выбираю. Следуя той же идее Refactoring -- Not on the backlog!, я могу наткнуться на тудушку в тех местах кода, которые я трогаю в процессе работы. Тудушки говорят о том, где был срезан угол. Если мне это мешает двигаться - я починю. Если не мешает, но я вижу что всё равно могу сделать лучше или просто сошлись звёзды, тоже починю. А нет - не судьба, будет лежать до следующего раза когда заглянет кто-нибудь ещё. Возможно долго пролежит, но и не страшно, работает - нет необходимости трогать. Может быть кусок кода с тудушками вообще будет удалён за ненадобностью, тогда усилия на починку не будут потрачены даром.
можно вооружиться идеей "работает - не трогай" и спокойно включать пререфакторинг в основную работу. См https://ronjeffries.com/xprog/articles/refactoring-not-on-the-backlog/
Да, можно. Проблема, опять же, в том, что пререфакторинг может спонтанно разрастись вне всякого контроля и отнять кучу времени - как у автора изменения, так и у ревьювера ("ловушка 2"). Поэтому я рекомендую более строго контролировать тягу к предварительным чисткам.
Каждое изменение несёт определённый риск. Про конфликты я уже писал. Согласование решений - ещё один риск. В командной работе не всегда же бывает так, что идеи всех участников получают одобрение. Чем больше отдельных решений содержит одно изменение, тем выше вероятность того, что кому-то что-то не понравится. К чему это приводит? К переделке или отмене отдельных изменений - то есть, к потере времени. Поэтому уменьшение числа решений, принятых в рамках одного PR, имеет смысл.
Предпосылка понятна, программисты в командах упрямые, на слово не верят, со слов процессы новые внедрять не станут. Им нужно на примере показать, продемонстрировать полезность, колеблющихся убедить, регулярно напоминать и подбадривать. И только потом, может быть, процесс заработает. Не знаю, бывают ли у кого-то другие команды.
К этому хочу сказать, что не всегда обязательно внедрять процесс на команду. Мы все работаем немного по-разному и друг к другу привыкаем, независимо от наличия и объёма гайдлайнов, правил и линтеров. Даже если в моей прежней жизни делали PR-ы с множеством мелких понятных коммитов, а в прежней не делают, я всё равно создаю такие PR-ы и включаю туда всё, что мне нужно, в том числе и префакторы. На случай, если ревьюерам становится страшно-очень-страшно, и они не понимают почему всего так много и каким боком там необязательные изменения, я каждый раз в описании (и при необходимости лично) сообщаю что нужно ревьюить по коммитам, для них я коммиты разбил, упорядочил и снабдил достаточными описаниями (привыкнув к интерактивному ребейзу, это не занимает много времени). То ли меня терпят, то ли действительно это не имеет большого значения, то ли практика эффективна, но жалоб за почти три года ещё не было. С другой стороны, подход явно не завирусился, поскольку применяю его я один, но для меня это не принципиально, главное что мне удобно и мои ревью проходят с нормальной скоростью.
Принципы непрерывного рефакторинга