Многие знакомы с ситуацией вида “откуда взялся этот фрагмент кода и зачем он нужен?”. Приходится тратить время и разбираться с уже рассмотренными другим коллегой деталями. Как сделать так, чтобы на это тратилось меньше времени? Для этого стоит уделить внимание описанию запросов слияния, также известных как “Pull Request” (PR, будет использоваться далее для краткости) или “Merge Request” (MR). Сразу оговорюсь, что далее речь пойдет именно об оформлении PR, т.е. о содержании пояснительной записки к нему и коммитах. Код я затрагивать не буду, потому как для каждого проекта существуют своя специфика и требования к его параметрам.
Примерный список моментов, которые рассматриваются в статье далее:
- Почему описание для Pull Request— это важно и полезно.
- Что должно содержать полезное описание Pull Request.
- Что может помочь с формулировкой описания Pull Request.
Почему описание для Pull Request — это важно и полезно
Для начала, попробуем понять, зачем вообще нужно уделять внимание описанию для PR. Представим процесс работы над проектом как путь, состоящий из коротких отрезков. Начало каждого из отрезков — это задача, шаги — коммиты, а конец — PR. Важность PR начинается с осознания того, что он является итогом вашей работы над задачей. В первую очередь, это коммиты с кодом, который вводит функционал, чинит баги или оптимизирует работу итогового продукта. Однако есть еще один момент — это контекст для вносимых изменений. Он подразумевает ваше обоснование выбранного способа реализации, выбранных инструментов, встреченных сложностей и дилемм. Предоставлять контекст для PR полезно, так как сама задача и ее обсуждение могут не давать всей полноты картины для понимания изменений. Хорошо, если обсуждение велось в трекере или, устно или в проектном чате, и было запротоколировано в виде коротких итогов. Если же нет, то потом придется отвечать на вопросы “откуда здесь этот код и зачем он тут нужен?”
Как ревьюер, не испытываю радости во время ревью PR, если приходится тратить дополнительное время и усилия на то, чтобы разбираться в коде и коммитах, потому как автор не удосужился составить описание. Автор изменений по итогам работы мог бы уделить некоторое время общему описанию PR еще на этапе “пока всё свежо в памяти”. Этим он бы сэкономил время и силы команде проекта, а именно:
- Ревьюерам — потому как им не придется гадать, чего вы хотели добиться, когда вносили изменения. Почему поступили именно так, а не иначе? Почему выбрали именно этот способ реализации или зависимости?
- Коллегам по проекту — найдя ваш коммит, они смогут найти и PR, где есть описание к внесенным изменениям и ссылка на задачу с обсуждениями. В итоге это поможет избежать багов, так как если ваш код нужно будет затронуть, при необходимости можно будет узнать базовые требования к его работе и уделить внимание соответствующим аспектам.
- Себе — вам не нужно будет отвечать на дополнительные вопросы, связанные с реализованными вами изменениями. В таких случаях всегда можно сослаться на описание, где важные моменты уже разъяснены. Это становится еще более актуальным по прошествии времени — так как через, скажем, полгода о внесенных изменениях помнить уже сложно, да и нет надобности, если они подкреплены содержательным описанием.
Вывод:
Описание PR не менее значимо, чем постановка задачи.
Оформлять итоги работы в описании для PR — это важно и полезно. Делая это, мы экономим силы и время на перспективу.
Что должно содержать полезное описание Pull Request
Теперь рассмотрим как следует подобрать описание для PR, чтобы сделать его полезным.
Заголовок
Начать следует с заголовка — он должен отражать суть изменений. Если есть соответствующая задача или тикет, то заголовок PR может с ними перекликаться. В таком случае следует добавить к заголовку префикс с номером задачи. Примеры:
SLI-001, SLI-002 - Support for reading input from STDINSLI-002 - Bugfix for memory leak when input is supplied via STDIN
Заголовок предпочтительно делать содержательным и достаточно коротким.
Связанные задачи
Следует указать задачи, которые решает или к которым относится ваш PR. Лучше делать это в виде маркированного списка со ссылками на тикеты в трекере. В MarkDown это может выглядеть примерно так:
# # Related tasks
- [SLI-001](http://my.issue.tracker/project/SLI-001)
- [SLI-002](http://my.issue.tracker/project/SLI-002)
Такой подход значительно упростит навигацию, позволяя найти все задачи и связанные обсуждения без особых усилий. Для задач, которые ведутся в GitHub и GitLab, есть специальные формулировки, которые позволяют закрыть их при слиянии PR — ими следует воспользоваться (смотреть здесь для GH и здесь для GL).
Зависимость от других Pull Request
Если ваш PR основывается на других открытых PR, которые должны быть предварительно рассмотрены, то их следует перечислить. Это касается и PR для используемых пакетов и зависимостей. Пример в MarkDown:
# # Depends on
- # 3 (другой открытый PR)
- dependen# 55 (открытый PR в пакете или зависимости)
Это позволит оптимизировать процесс рассмотрения PR и сэкономить время ревьюерам. Если ваш PR ни от чего не зависит, секцию следует пропустить.
Предпосылки
Эта секция может быть посвящена тому, что предшествовало вносимым изменениям. Перечень того, что следует изложить приведён ниже.
- Если вы делали выбор между возможными пакетами и выбирали более подходящий, то стоит привести ссылки на каждый пакет, затем описать его плюсы и минусы при решении задачи. Следует также изложить итоговую причину, по которой вы выбрали один из них для ввода в проект.
- Если вы исследовали какую-то тему и искали наиболее подходящие пути решения задачи, то стоит привести ссылки на статьи, которые помогают в исследовании темы. Также можно описать ключевые тезисы, которые способствуют пониманию последующих изменений.
- Если вы обнаружили, что вам мешает какой-то существующий код, то следует изложить ваши доводы. Если какая-то функция плохо оптимизирована или плохо реализована, и вам придётся её рефакторить, точно стоит упомянуть почему вы решили её затронуть более, чем ожидалось ранее.
- Краткие итоги обсуждений с командой, которые позволят понять, почему приняты те или иные решения.
Пример может выглядеть так:
# # PremiseAs we have grammar description and access to an assembly code even from AST, I considered to use PEG-based parser to generate custom AST from assembly code, then use adapter to make it compatible with current ASTReader processing logic. As prerequisites I’ve looked through the following packages:
- https://github.com/navstev0/nodebnf (npm) — I tried and in the end came to the conclusion, that it is not so stable to be used for our needs. Also, lacks proper documentation.
- https://github.com/lys-lang/node-ebnf (npm) — Has proper documentation and TypeScript definitions. Also, has a useful online sandbox. Chosen for the further implementation.
Эта секция служит для общих случаев, когда есть необходимость в рамках разумного изложить любые сведения, которые следует знать до того, как ревьюер (или любой другой коллега) приступит к изучению изменений с вашими правками. Если же такой необходимости нет, то секцию следует пропустить.
Изменения
В секции следует списком перечислить вносимые изменения. Если вы до этого подробно описывали вносимые изменения в сообщениях коммитов, то можно взять текст из них. Если вы считаете, что какие-то сообщения требуют дополнительного пояснения, то вы можете указать важные сведения в виде подпунктов.
# # Changes- Added dependency [ebnf](https://www.npmjs.com/package/ebnf).
- Cleaned **package.json** from some obsolete commands. Introduced `ebnf:update` to copy BNF-files when post-install occurs.
- Introduced **src/ebnf** directory with components to parse code strings by BNF definitions.
- Tweaked configuration to be able to work with a new set of components.
- Introduced tests to cover new functionality.
Чем следует руководствоваться при формулировании содержимого секции? Подробный список приведен ниже.
- Часть изменений доступна при просмотре изменений в файлах. Поэтому не стоит “микрить” — GIT это сделает за вас. Лучше обобщить изменения, изложив их смысл. Например:
- Обновлены версии зависимостей, потому что они сильно устарели и аудит показывал, что там есть уязвимости.
- Удалены файлы в каталоге X, потому как работавший с ними компонент был заменен на другой. Они оказались более не нужны.
- Внесены изменения в метод, потому что низкоуровневый API изменился. Из-за этого метод больше не работал.
- При обосновании действий, связанных с зависимостями, не стесняйтесь использовать ссылки на релизы, слияния багфиксов или фич — это очень полезно в будущем.
- Если вводите новый класс, функцию, метод или другую рутину, то лучше описать ее предназначение (а еще лучше сделать это в док-блоке перед ее заголовком).
- Старайтесь уделять чуть больше внимания именно тем действиям, которые невозможно прокомментировать в файлах с исходным кодом: перемещение файлов, удаление, программное изменение (те же настройки зависимостей, к примеру).
Отдельно стоит обратить внимание на пару возможностей:
- Для наглядности не стесняйтесь ссылаться на сегменты кода, который имеете в виду. Как это делается, можете прочитать здесь для GitHub (для GitLab не нашел инструкции, буду рад, если кто-то поделится).
Проблемы
Если вы в чем-то сомневаетесь или замечаете противоречия, то лучше изложить их в этой секции. Не стесняйтесь изложить моменты, в которых вам бы не помешал совет от коллег. Вот краткий список моментов, которые могут стать поводом для упоминания:
- Вы недавно на проекте или еще не опытны в использовании какой-либо технологии. Пока что сделали “как получилось”, но не знаете, верно ли вы поступили.
- Вы обнаружили проблему, которую не удаётся устранить, и вам бы не помешал совет от коллег.
- Вы столкнулись с нехваткой времени или дополнительными сложностями и из-за этого оставили часть функционала нереализованным, поместив в код “заглушку”. Вам нужен совет.
- Вы можете предложить другой более эффективный подход к реализации, но не знаете, стоит ли его применить сейчас или отложить на более поздний этап разработки (в зависимости от обстоятельств).
Вот возможный пример в MarkDown:
# # Concerns
It appears that the compiler is trimming spaces and reducing indentation within assembly code strings in `Assembly` AST node. This causes inability to generate proper `src` attributes for custom AST by simple math. Also, this is not fixable, because these dependencies are already released and can not be patched. Need to find another way to link `AssemblyIdentifier` to a top-level `VariableDeclaration` node. Currently, any `AssemblyItem` node will have the same `src` as parent `Assembly` node. Maybe I'm missing something... I would appreciate any suggestions that will help in solving the situation.
Если вы не обнаруживаете проблем, то не следует тратить время на данную секцию.
Примечания
Эта секция служит для перечисления моментов, которые не столь очевидны. Например, ввели новую команду сборки, которая производит копирование файлов, либо использовали какой-то функционал c “волшебными цифрами”. Лучше упомяните об этом заранее, потому что об этом всё равно спросят рано или поздно. Вот примеры:
# # Notes
- This PR changes major package API, which would result in a backward compatibility (BC) break.
- When changes applied to grammar files, developer must run `npm run ebnf:update` to copy updated grammar files to **dist/**. Otherwise, components will not react to any changes. All grammar files are static assets, so they are not compiled or handled by **tsc** in any way.
- For some reason `process.stdin.fd` is not available in type definitions for the current node version. It seems that we should upgrade at some point. I resolved this via magic `fse.readFileSync(0, { encoding: "utf8" })` (as `0` should point to `STDIN` for each process) and left a comment with the reasoning.
Что может помочь с формулировкой описания Pull Request
Приведу несколько шагов, которые позволят вам понять, как именно подобрать содержимое для описания PR.
Обратите внимание на то, что GitHub и GitLab предоставляют возможность создать шаблон для пояснительных записок к PR (инструкции приведены здесь для GH и здесь для GL). Посоветуйтесь с командой: может быть, стоит систематизировать оформление заранее.
На стадии написания кода задумайтесь о том, что стоит писать в сообщениях коммитов. Грамотно сформулированные сообщения коммитов — один из лучших источников для секции изменений. Не забывайте упоминать код тикета из трекера. Современные трекеры (Atlassian Jira, к примеру) способны собирать данные с подключенных проектных репозиториев и формировать ссылки на относящиеся к ним коммиты и PR.
Перед созданием PR, поставьте себя на место ревьюера. Создайте PR только с двумя секциями “Tasks” и “Changes” с пометкой “Reserved”. Затем проведите ревью сами себе. Это поможет взглянуть на изменения со стороны. Учтите, что с вашими изменениями после слияния может знакомиться и менее опытный коллега. Задайте себе вопрос: “Возможно, чтобы не тратить время в дальнейшем, стоит дать дополнительные пояснения в описании?”. Подумайте о том, какие вещи могут вызывать вопросы в перспективе. Если в работе над проектом задействовано более двух человек, то коллеги могут начать задавать вопросы. Если есть возможность ответить на часть из них заранее — это хороший повод расширить описание PR.
Далее, попробуйте немного отстраниться от контекста и подумайте, как его лучше донести до читателя. Проект может быть передан другим разработчикам. В этом случае будет необходимо писать инструкцию. Задайтесь вопросом “Не лучше ли мне сейчас изложить важные вещи, пока всё свежо в памяти?”.
Вспомните, обсуждали ли вы с коллегами какие-то важные моменты реализации устно или на других ресурсах. Если обсуждения имели место быть, то потратьте немного времени, чтобы описать краткие итоги. Не стесняйтесь и указывайте ссылки на полезные статьи, которые вы читали и которые помогут понять логику изменений или принятых решений.
Подумайте, стоит ли оформить PR нагляднее. Обратите внимание на возможность использования “меток” (Labels), которые помогут понять что ваш PR содержит особенности: изменения в зависимостях, обратную несовместимость, исправление бага или новую функциональность. Метки повышают наглядность и помогают с поиском при необходимости (например, поиск всех исправлений багов при такой разметке делается по одному щелчку мыши). GitHub и GitLab позволяют использовать весь арсенал форматирования MarkDown (ознакомьтесь с инструкцией для GH и для GL). Средства оформления помогут облегчить читабельность и упростить понимание многих вещей. Если изменения коснулись графического интерфейса, то вы сможете обратить на это внимание при помощи вставки снимков экрана “до” и “после” изменений.
Помните, что хорошо описанные PR помогают вести дальнейшую документацию по проекту, в частности “список изменений” (ChangeLog), “нововведения” (Release Notes), “вводную инструкцию” (ReadMe). Для первых двух достаточно указывать ссылки на PR, а для инструкции можно скопировать и вставить часть контента. Составляя содержательное описание PR, вы поможете себе и коллегам не тратить время на повторное перечитывание кода перед релизом новой версии — можно будет обойтись проходом по принятым с момента последнего релиза PR.
Заключение
Данной статьёй я старался донести, что содержательный и хорошо оформленный PR — это проектный документ, по ценности не уступающий изначально поставленной задаче. Если вы писали код несколько часов, то не будет лишним выделить полчаса на то, чтобы подвести итоги работы. Приучив себя к культуре оформления PR, вы сильно упростите жизнь себе и команде проекта, а еще сэкономите кучу времени на ревью и обсуждениях.
Примеры
Приведу несколько PR для проекта с открытым исходным кодом, которые могут послужить в качестве наглядных примеров:
Дополнительная литература
Предложу несколько полезных статей по теме:
Ранее статья была опубликована тут.
