Перейти к содержанию

Код-ревью

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

Цели

  • Найти ошибки
  • Уловить неочевидные последствия подхода — сделает ли этот PR будущий код более сложным или более проблемным.
  • В ситуациях, когда что-то было закодировано без обсуждения, проверка кода служит проверкой работоспособности, чтобы убедиться, что используется правильный подход.
  • Укажите последствия PR для тех частей Glarus BI, которые PR не затрагивает.
  • Укажите места, где был использован хороший подход или стиль. Обзоры кода — это не праздник ненависти. Если PR не является совсем уж ужасным, должно быть примерно одинаковое количество положительных и отрицательных комментариев.

Настрой на проверку кода

Ваша основная цель как рецензента — подстраховать и не допустить вливания плохого кода в основную ветку. Определение «плохого» очень субъективно, зависит от контекста и будет меняться со временем и зрелостью продукта.

Когда вы обнаружите явные ошибки, найдите время, чтобы объяснить, почему вы считаете их ошибками.

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

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

Настрой на принятие внешней проверки

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

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

Если кто-то поставит сильно негативную оценку вашему коду, будьте терпеливы. Попытайтесь понять, почему они думают, что PR ошибочен. Подойдите к разговору с намерением улучшить PR, а не защищать свой подход. Вы не получаете баллов за то, что лучше ведете дебаты, но вы получаете баллы за лучший код и лучший продукт, независимо от того, откуда пришло вдохновение или идея.

Процесс

  • Каждый PR значительной сложности должен быть одобрен как минимум одним другим инженером в команде (или @salsakran) для слияния.
  • Добавьте людей, которые, по вашему мнению, должны просмотреть ваш PR, в список уполномоченных на ревью PR. Рецензент может удалить себя после того, как просмотрит его или решит, что он не является подходящим рецензентом.
  • Код, влияющий на работу других инженеров, должен проверяться этими инженерами.
  • :+1: по умолчанию «Я согласен с этим»
  • :+0: (это я выдумал) означает: «Я не в восторге от этого, но другие говорят, что «+1», поэтому код можно принять
  • :-1: это жёсткое вето. Это следует использовать с осторожностью в обычных PR, и только для не оттестированных вещей, вопиющих нарушений руководства по стилю или ошибочных предположений, от которых зависит другая часть базы кода.
  • Если вы делаете крупное изменение, не обсуждая дизайн или не обсуждая последствия с другими инженерами, чья работа может быть затронута, вам следует ожидать :-1:, а не зацикливаться на переработке спорных разделов.
  • Любой PR, который имеет :-1:, НЕ МОЖЕТ быть объединен, пока конфликты не будут разрешены.
  • Владелец PR и человек, использующий :-1:, должны устранить обсудить и устранить свои разногласия.
  • Если возникает полный тупик, то @salsakran подает решающий голос. Такие тупики должны быть крайне редкими.

Обратите внимание, что эти :+1:, :+0: и :-1: должны быть явно указаны в комментарии, а не как реакция на основное описание PR на github. Изменение с :-1: на :+1: также должно быть явно указано в комментарии.

Сроки

  • PR для ошибок с высоким приоритетом должны быть проверены, как только они станут доступны.
  • PR для ошибок, запланированных к конкретной дате может подождать несколько дней.
  • Если в PR нет :+1:, создатель PR обязан обратиться к проверяющим и добиться проверки кода. Другими словами для принятия кода, PR должен иметь :+1:, и если проверка кода не была проведена, то создатель PR должен найти рецензентов.
  • Если есть :-1: и нет четкого решения, то создатель PR, и тот, кто поставил :-1: должны найти время в течение следующего дня или двух, чтобы обсудить проблему и обсудить, как её решить.
  • В случае отсутствия движения по PR с :-1: через неделю вмешается @salsakran.

Как улучшить качество проверки кода

Сводную информацию по исследованиям проверки кода см. Как проверка кода работает (и как не работает) в реальном мире.

Что авторы PR могут сделать, чтобы проверка кода стала лучше

  • Направляйте рецензентов, комментируя важные разделы кода.
  • Если вам нужен чей-то опыт/мнение, тегните (отметьте) этого человека
  • Улучшите PR-описания, используя Примечания и предупреждения - эти инструменты будут эффективны, если вы хотите выделить определённую часть информации.

Что рецензенты PR могут сделать, чтобы сделать проверку кода лучше

  • Обратите особое внимание на тестовые файлы. Помните о нашей склонности придавать им меньшую значимость и прилагайте сознательные усилия для их тщательного анализа.
  • Начните с файла, наиболее важного для изменения, а не с первого по алфавиту в PR.
  • Если вы чувствуете, что недостаточно ориентируетесь в сути запроса - попросите автора предоставить более подробную информацию/лучшее описание.
  • Если изменение не является тривиальным, проверьте эту ветку и локально протестируйте Glarus BI с включенными изменениями. Убедитесь, что всё работает, как ожидалось.