Контрольный список проверки PR

Solidity компилятор — критически важная часть инфраструктуры в экосистеме Ethereum. По этой причине наш процесс проверки довольно строг, и все PR должны соответствовать определённым ожиданиям качества и рекомендациям.

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

Перед отправкой PR:

— [ ] Есть ли у вас другие открытые PR? Работа над PR не завершена, пока он не будет объединён или закрыт. Наша способность к проверке ограничена, поэтому мы требуем, чтобы внешние участники работали не более чем над одним PR одновременно. Если ваш PR не проверяется, вы можете привлечь к нему наше внимание на канале #solidity-dev. Если их не запрашивали, мы собираемся закрыть любые лишние PR, оставив открытым только самый ранний. Вы можете повторно открыть их по одному, когда ваш текущий PR будет завершён.

— [ ] Готов ли вопрос к работе? Если у вопроса нет метки желательности (nice to have, should have, must have eventually, must have, roadmap), мы ещё не решили, стоит ли его реализовывать. Если вопрос имеет метку needs design, мы ещё не определились, как его следует реализовать. good first issue candidate означает, что вопрос потенциально станет хорошим первым вопросом со временем, но в данный момент он ещё не готов к работе.

— [ ] Это критическое изменение? Критические изменения должны основываться на ветке breaking, а не на ветке develop.

— [ ] Действительно ли PR решает проблему? Упомяните номер вопроса в описании PR. Если PR полностью решает его, используйте форму Fixes #<номер вопроса>, чтобы GitHub автоматически закрыл вопрос. Не включайте номер вопроса в заголовок PR, название ветки или описание фиксации.

— [ ] При отправке PR из форка создайте ветку и дайте ей описательное имя. Например, fix-array-abi-encoding-bug. Не отправляйте PR непосредственно из ветки develop вашего форка, так как это затрудняет перебазирование и получение новых изменений.

— [ ] Зависит ли PR от других PR? Если PR имеет зависимости, укажите их жирным шрифтом в описании. Избегайте основывать PR из форков на ветках, отличных от develop или breaking, потому что GitHub закрывает их, когда базовая ветка объединяется. Делайте это только для PR, созданных непосредственно в основном репозитории.

— [ ] Обновляет ли PR тестовые ожидания в соответствии с изменённым кодом? Если нет, ваш PR не пройдёт некоторые из заданий _soltest_ в CI. Во многих случаях ожидания можно обновить автоматически:

• cmdlineTests.sh --update для тестов командной строки.
• isoltest --enforce-gas-cost --accept-updates для основанных на soltest тестов. Если ваш PR влияет на стоимость газа, необходим дополнительный запуск isoltest --enforce-gas-cost --optimize --accept-updates, чтобы обновить ожидания по газу с включённым оптимизатором. Просмотрите обновлённые файлы перед фиксацией. Верны ли ожидания и служат ли обновлённые тесты своей цели?

Заброшенные PR:

— [ ] Ответственный за PR всё ещё отвечает? Если в PR не было активности от отправителя в течение последних 2 недель (несмотря на получение отзывов и наших запросов), мы считаем его заброшенным.

— [ ] Легко ли завершить заброшенный PR или он актуален? Примените метку takeover, если PR можно завершить без значительных усилий или это действительно нужно сделать прямо сейчас. В противном случае закройте его. Его всё равно можно будет взять позже или повторно открыть отправителем, но до тех пор мы не должны отвлекаться на него.

Быстрый обзор:

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

Лёгкий обзор должен быть сосредоточен на следующих трёх областях:

• Есть ли очевидные ошибки? Проблемы стиля, плохие практики, легко обнаруживаемые баги и т. д.
• Чего-то не хватает? Тестов (правильного типа), документации и т.д. Решает ли это проблему в целом?
• Это правильное решение? Есть ли лучшие способы сделать это? Действительно ли изменение необходимо?

Если ответы на вышеперечисленные вопросы «Да, Да, Нет», поблагодарите автора за его усилия и закройте PR.

Стиль кодирования и хорошие практики

• Следует ли PR нашему стилю кодирования?

Надёжность

• Используйте утверждения без ограничений. Если вы уверены, что ваше предположение не будет нарушено, докажите это с помощью solAssert().
• Проверяйте входные данные и обрабатывайте ошибки. Обратите внимание, что утверждения — это не проверка.

Читаемость

• Выбирайте хорошие имена.
  • Является ли имя простым для понимания? Чувствуете ли вы необходимость вернуться к определению и напомнить себе, что это было, когда вы видите его?
  • Недвусмысленно ли имя в контексте, где оно используется?
  • Избегайте сокращений.
• Исходные файлы, классы и публичные функции должны иметь докстринги.
• Избегайте дублирования кода. Но не фанатично. Минимальное количество дублирования допустимо, если это способствует читаемости.
• Не оставляйте за собой мёртвый или закомментированный код. Вы всё ещё можете видеть старый код в истории. Если у вас действительно есть веская причина сделать это, всегда оставляйте комментарий, объясняющий, что это такое и почему он там.
• Помечайте хаки как таковые. Если вам нужно оставить временный обходной путь, обязательно включите комментарий, который объясняет, почему и при каких обстоятельствах его можно удалить. Желательно связать с проблемой, о которой вы сообщили вышестоящему руководству.
• Избегайте очевидных комментариев.
• Включайте комментарии, когда читателю может потребоваться дополнительный контекст для понимания кода.

Коммиты и PR

• Избегайте скрывать функциональные изменения внутри рефакторинга. Например, при исправлении небольшой ошибки или изменении видимого пользователем поведения поместите изменение в отдельный коммит. Не смешивайте его с другим изменением, которое переименовывает вещи или переформатирует код вокруг, делая сам по себе фикс трудно идентифицируемым.
• Когда это возможно, разделяйте рефакторинг или несвязанные изменения на отдельные PR. Меньшие PR легче и быстрее просматривать. Разделение рефакторингов помогает сосредоточиться на основной сути PR.

Общие ловушки

Следующие пункты охватываются стилем кодирования, но встречаются так часто, что стоит выделить их здесь:

• Всегда инициализируйте типы значений в определении, даже если вы уверены, что назначите их позже.
• Используйте стиль «east const». То есть T const*, а не const T *.
• Сохраняйте согласованность отступов. См. наш .editorconfig.
  • Табы для C++. Но используйте их только для отступа. Любой пробел позже в строке должен состоять из пробелов.
  • 4 пробела для большинства других типов файлов.
• С осторожностью используйте auto. Используйте его только тогда, когда фактический тип очень длинный и сложный или когда он уже используется в том же выражении.
• Отступы фигурных скобок и круглых скобок таким образом, чтобы было ясно вложение.
• Используйте using namespace только в .cpp файлах. Используйте его для std и наших собственных модулей. Избегайте ненужного префикса std:: в .cpp файлах (кроме std::move и std::forward).
• Используйте циклы на основе диапазона и деструктуризацию.
• Включите все заголовки, которые вы используете напрямую, даже если они неявно включены через другие заголовки.

Документация

• Обновляет ли PR соответствующую документацию?

Стиль документации и хорошие практики

• Используйте двойные обратные кавычки в RST (``x``). Предпочитайте одинарные обратные кавычки в Markdown (`x`), но обратите внимание, что двойные обратные кавычки также допустимы, и мы используем их в некоторых случаях по устаревшим причинам.
• Всегда начинайте новое предложение с новой строки. Таким образом, вам не придётся переписывать окружающий текст, когда вы переписываете... Сделайте шаг назад

• Вы полностью понимаете, что делает PR и почему?
• Вы уверены, что код работает и не нарушает несвязанную функциональность?
• Это разумный способ достичь цели, указанной в задаче?
• Код простой? Не приводит ли PR к значительному усложнению, которое может стать источником будущих ошибок?
• Код эффективный? Нет ли в PR серьёзных узких мест, влияющих на производительность?
• Не вносит ли PR критические изменения сверх того, о чём договорились в рамках задачи?

Заключительные проверки перед слиянием

• PR перебазирован поверх ветки develop (или breaking, если это критическое изменение)?
• Все проверки CI пройдены?
  • Обратите внимание, что у нас есть несколько заданий, которые могут случайно завершиться неудачно из-за внешних факторов, особенно внешние тесты (с _ext_ в названии). Если они завершаются неудачно, перебазируйте на последнюю версию develop (или breaking) и попробуйте запустить их повторно. Также обратите внимание, что не все эти проверки обязательны для слияния PR.
• Если изменение видно пользователям, есть ли у PR запись в журнале изменений?
  • Запись в журнале изменений находится в нужном разделе? Убедитесь, что вы переместили её вверх, если недавно был релиз.
• История коммитов простая и понятная?
  • Каждый коммит должен быть самодостаточным логическим шагом, ведущим к цели PR, без возвратов назад и вперёд. В частности, исправления должны быть объединены в коммиты, которые они исправляют.
  • Не включайте в свою ветку какие-либо коммиты слияния. Пожалуйста, используйте перебазирование, чтобы поддерживать актуальность с базовой веткой.
• Правильно ли помечен PR?
  • Используйте метку внешний вклад, чтобы пометить PR, не исходящие от основной команды.
  • Если PR зависит от других PR, используйте имеет зависимости и установите базовую ветку соответствующим образом. — Метки вроде документация или оптимизатор полезны для фильтрации PR.