Правильное ревью патча по рецепту webchink

26.09.2017

Зображення видалено.
Это вольный перевод достаточно старой статьи Webchink "Diaries of a Core Maintainer #5: The 6-pass patch review", о том, какую стратегию она использует (использовала) для ревью патчей ядра.

Если вы не читали данную статью ранее или другие статьи по этой теме, этот перевод поможет вам понять:

  • почему же ваш патч всё никак не может пройти в ядро;
  • что сделать, чтобы ваш патч поскорее туда попал;
  • как делать качественные ревью патчей;

Перевод

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

  • острый глаз
  • пытливый ум
  • способность сопереживать людям новым в drupal и почувствовать себя на их месте

Уровень 1. Coder Module
Вы делаете ту же работу, что и coder module. Вы быстро просматриваете код на соответствие кода и комментариев стандартам форматирования. Это хорошее начало ревью патча т.к. позволяет так же понять что делает патч.

Уровень 2. Снизу-вверх
Вы быстро читаете код патча снизу вверх и оцениваете, насколько вам понятно, какую работу выполняют те или другие участки кода. При этом, вы не поддаетесь соблазну читать комментарии.
Это отличный способ для выявления вещей, таких как:
some_function(NULL, TRUE, array(), $booger, 'aardvark');
Например, в этой строке непонятно для чего предназначена функция и какие параметры она принимает и очевидно, она требует некоторого рефакторинга:

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

Уровень 3. Только комментарии.
Теперь, когда вы уже дважды просмотрели код, скорее всего вы начали примерно понимать что делает патч.
Задача на этом этапе - понять, отражают ли комментарии ваше понимание кода и выявить места в которых не всё понятно.
Вы проверяете содержание комментарием, и не читая кода (теперь наоборот), снова стараетесь только на основе комментариев понять, что делает патч

Уровень 4. Читаем issue.
Да, всё верно. Только сейчас мы читаем саму issue.
Почему это шаг 4, а не 0?
Потому, что пользователи, которые будут применять этот патч не будут иметь такой роскоши, как чтение всей линии обсуждения в issues патча, и понимания цепочки решений, которая сделала патч таким, каким он есть.
Когда вы прочитали комментарии к issues до ревю, это дает вам бэкграунд, которого не будет у пользователей патча и это будет не честно с вашей стороны по отношению к этим людям, т.к. вы можете пропустить в патче вещи, которые вы не пропустили бы, если бы не читали issue.
Кроме того, чтение issue до ревью, не позволяет вам составить “свежий взгляд со стоны” и вы можете допустить те же ошибки, которые были допущены в обсуждении issue, если они есть.

Итак, читая issue после того, как вы уже сделали частить ревью, ваша задача - проследить, все ли ключевые моменты, возникшие при обсуждении патча нашли своё отражение в коде и комментариях:

  • решения относительно имплементации
  • комментарии относительно плохо читаемых участков кода, если они есть в issue
  • если какой-то функционал патча был прокомментирован как полезный

Уровень 5. Это работает ?
Сейчас, и только сейчас наконец можно применить патч и проверить, действительно ли он делает, то что должен.
Следует подумать, о неочевидных вещах, как и каким образом он может повлиять на другие части системы.
Например, если данный патч изменяет что то в системе регистрации пользователей, можно предположить что эти изменения могут повлиять на систему авторизации, т.к. эти вещи связаны, и система авторизации, в данном случае, так же должна быть протестирована. А если вы владеете настоящим Testing-Fu, то можно проверить как патч повлиял на систему авторизации/регистрации по OpenID, т.к. ошибки которые могу возникнуть здесь, иначе долго оставались бы невыявленными.

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

Уровень 6. Собираем всё воедино.
Сейчас, со всем знанием и заметками о патче, нужно пройти по всем шагом еще раз, и набросать новые заметки.
Вероятно, к тому времени, как вы закончите 2й круг, ваш текстовый файл с заметками по патчу вырастет до 60 Мб и будет содержать множество вопросов и обнаруженных недостатков, достаточно много, чтобы отправить патч на доработку ;)

Оригинал статьи:
webchick.net/6-pass-patch-reviews

Полезные ссылки:
Начать делать ревью патчей

Стандарты кода
Модуль Coder
Проект testbot и его описание