Здравствуйте, Pzz, Вы писали:

Pzz>В реальном мире получаешь 10 комментов про то, что хвостик у запятой не той формы, и редко-редко получаешь коммент по делу.

Это называется имитация деятельности, и происходит потому что ревьюеры не понимают целей ревью.
Видел такое, да, печалька.

Здравствуйте, Буравчик, Вы писали:

Б>Какие комментарии в код-ревью вы считаете "по делу", а какие нет?
Б>Какими должны быть правильные комментарии к PR/MR/т.п.?

Б>(Навеяно темой в "коллеги улыбнитесь")

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

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

Часто встречал недовольство предложением, а как правильно написать, конечно, никто не удосуживается.
В итоге и сам не понимаешь, что не нравится, и не знаешь как сделать "правильно".

Здравствуйте, Буравчик, Вы писали:

Б>Какие комментарии в код-ревью вы считаете "по делу", а какие нет?
Б>Какими должны быть правильные комментарии к PR/MR/т.п.?

— грубые ошибки "в таком виде функцию нельзя покрыть юнит-тестами, а вот в таком — можно"
— баги
— секурити проблемы
— отхождение от принятого дизайна/архитектуры — вопросы "почему конфиг используется напрямую"
— где тесты "вот этой штуки"
— непонятные места — вопросы "а что это за константа"

до кучи — накидывать тикеты на те части, что требуют доработки. Это проще, чем переписываться неделями. В пр нужно просто указать, что создан тикет "исследовать, что будет в случае xxx при условии yyy"

стили, запятые, форматирование, линтинг — это всё мусор, решается автоматикой

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

Здравствуйте, Буравчик, Вы писали:

Б>Какие комментарии в код-ревью вы считаете "по делу", а какие нет?
Б>Какими должны быть правильные комментарии к PR/MR/т.п.?

Б>(Навеяно темой в "коллеги улыбнитесь")

missed null check, broken flow (details), hidden bug (details). Ignored exception. — Это то, что по делу.

Не по делу — "А как это вообще работает? Мне непонятно. Переделать! Need Work!!!!

Здравствуйте, Doom100500, Вы писали:

D>Не по делу — "А как это вообще работает? Мне непонятно. Переделать! Need Work!!!!

Кстати, "А как это вообще работает? Мне непонятно" обычно указывает, что в коде не хватает комментариев.

Задаешь этот вопрос, зачем это здесь? — и появляется либо описание магии, либо магия заменяется на что-то более простое. В обоих случаях код становится лучше.

Здравствуйте, Буравчик, Вы писали:

Б>Здравствуйте, Doom100500, Вы писали:

D>>Не по делу — "А как это вообще работает? Мне непонятно. Переделать! Need Work!!!!

Б>Кстати, "А как это вообще работает? Мне непонятно" обычно указывает, что в коде не хватает комментариев.

Это значит, что был пропущен этап "Design Review".

Б>Задаешь этот вопрос, зачем это здесь? — и появляется либо описание магии, либо магия заменяется на что-то более простое. В обоих случаях код становится лучше.

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

Здравствуйте, Alekzander, Вы писали:

A>Здравствуйте, Буравчик, Вы писали:

Б>>Какие комментарии в код-ревью вы считаете "по делу", а какие нет?
Б>>Какими должны быть правильные комментарии к PR/MR/т.п.?

Б>>(Навеяно темой в "коллеги улыбнитесь")

A>Вообще считаю код-ревью плохой практикой. Комментарии надо собирать, когда код ещё пишется, а не когда код уже написан. Например, тимлид может несколько часов в неделю посидеть с каждым. Или люди могут периодически работать вместе, показывать друг другу свой код. А когда код написан, и его надо переписывать, это приводит к прохождению через все стадии гнева/отрицания/торга/принятия, а это отнимает силы и мотивацию. Жаль, что мало команд умеет обходиться без этого всего.

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

Здравствуйте, Alekzander, Вы писали:

A>...А когда код написан, и его надо переписывать, это приводит к прохождению через все стадии гнева/отрицания/торга/принятия, а это отнимает силы и мотивацию. Жаль, что мало команд умеет обходиться без этого всего.

Ну так не надо, конечно. Порефакторить после ревью — да, возможно, но не переписывать. Если код уже написан, и делает то, что надо нужным образом (типа хранение в таблице БД, а не файлах), то переписывать его не нужно — доработать намного проще.

Здравствуйте, Alekzander, Вы писали:

A>Вообще считаю код-ревью плохой практикой.

охотно бы не согласился, если не поработал бы в компании без code review.
На практике получается вполне недурно. Ну реально блин, 95% времени ты отбиваешься от вкусовщины ревьювера. Да, остальные 5% — дельные замечания, но стоит ли оно потраченного времени? Реальные баги всё равно эффективнее выловит система тестирования, чем разработчик просто пробежав глазами. Посчитайте сколько уходит времени на ревью у вас. У меня может уходить x3 от времени собственно разработки. В компании без ревью я прям был приятно удивлён как быстро проходит процесс принятия MR.
Похоже на ситуацию с остальными практиками agile. Почти все превратились в бюрократических монстров, которые уже давно не служат главной цели — экономить время, а работают только на поддержание бюрократического аппарата в виде менеджеров и бесконечных митингов.

Здравствуйте, Буравчик, Вы писали:
Б>Какие комментарии в код-ревью вы считаете "по делу", а какие нет?
Б>Какими должны быть правильные комментарии к PR/MR/т.п.?

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

Здравствуйте, sergii.p, Вы писали:
SP>На практике получается вполне недурно. Ну реально блин, 95% времени ты отбиваешься от вкусовщины ревьювера. Да, остальные 5% — дельные замечания, но стоит ли оно потраченного времени? Реальные баги всё равно эффективнее выловит система тестирования, чем разработчик просто пробежав глазами.
Ревью призвано выявлять не баги, а говнокод. Цель — минимизация накопления техдолга.
Потому что без ревью есть искушение сделать всё тяп-ляп — потом разберёмся. Или тупо по незнанию залудить что-нибудь лишнее, вроде дубликата функции, которая уже есть в проектной библиотеке. Тесты проходят — алга в продакшн.
А вот потом, через полгодика, кто-то смотрит на этот код (например, в процессе добавления новой фичи), и видит, что всё как-то кривовато сделано.
Но времени переписывать нормально нет — это ж надо погрузиться в код; вспомнить что там, как, и зачем; аккуратно переписать; перетестировать. Даже если это делает автор того самого кода. Ещё веселее, если это чужой код — поди тут отличи "чувак не знал, что это поле можно не обнулять" от "чувак знал, что это поле нельзя не обнулять".
Вот уже и день прошёл — а собственно то, ради чего в этот код зашёл, ещё даже не началось.
И никто не стоит над душой, не давит "увидел бардак — прибери". Так что велики шансы не рефакторить, а костылить. И так постепенно код превращается в неподдерживаемый mess, и возникают вопросы вроде "как убедить менеджмент выделить время на рефакторинг говнокода и прочий техдолг".

Перефразируя известный мем, две недели рефакторинга могут сэкономить вам целых четыре часа код-ревью.

Здравствуйте, Sinclair, Вы писали:

S>Ревью призвано выявлять не баги, а говнокод. Цель — минимизация накопления техдолга.

так определение говнокода — это и есть вкусовщина. Одному кажется, что код слишком тесно связан и надо добавить абстракций, другой возразит что это будет обычный оверинженеринг. В итоге ломается куча копий а движения нет.
И посыл что техдолг накапливается только из-за говнокода, мне кажется ошибочным. Техдолг возникает в основном из-за неопределённых требований. На первом этапе зачастую надо сделать быстрый прототип, быстро нарастить функционал, быстро показать заказчику. Поэтому избирается максимально простой дизайн без лишних наворотов. Но затем то требования растут по мере успешности проекта. И тут то и образовывается техдолг. То есть это объективный процесс и напрямую не связан с квалификацией разработчиков и качеством кода.