Pull to refresh

Comments 172

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

Можно минимизировать негативный эффект, если преред созданием пулреквеста прибраться в коммитах. Когда каждая простая мысль выносится в отдельный коммит, то перед ревью можно коммиты про одно и тоже объединить в один (ребейз + перенос коммита + сквош). Это часть саморевью.

Как и в любом творческом процессе надо найти компромисс
Почему бы не отдать на ревью просто итоговый код? В конце-концов смысл кода не в отдельно взятых «мыслях», а в той фиче, которую он реализует.
При наличии отдельных коммитов посмотреть итоговый код не проблема, можно смотреть изменения не по коммитам, а целиком. Но наличие коммитов, оставляет возможность посмотреть как шла разработка. Если изменений много, то можно делать ревью не за раз, а с перерывами. Но для этого каждый коммит должен быть логически завершенным кусочком.
UFO just landed and posted this here
Действительно, пользу приносит только пуллреквест целиком.

Разбиение по коммитам это один из способов донести до ревьювера мысль. Рассказать по шагам. Акцентировать внимание на деталях, которые легко пропустить в общем потоке.

Иногда, мысль, которая выражена в пуллреквесте, не укладывается у меня в голове. Она большая и сложная. А большие и сложные задачи я обычно декомпозирую на маленькие и простые. Перенёс эту практику на пулреквесты и мне стало удобнее.
Я на работе выработал примерно такую же практику по всем пунктам, но сейчас принуждаю всех ещё добавлять схемы архитектуры в issue. Очень облегчает понимание задачи и её реализацию. Используется Gitlab и встроенный в неё Mermaid. Он не очень умный, но небольшие схемы хорошо отрисовывает. Сложные схемы приходится разбивать на части.

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

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

Использование --force в стандартном flow уже запашок. Я понимаю, если надо исправить ошибки. Но вот в повседневном использовании это явно показатель нехорошего. Учитывая рутинность можно легко допустить ошибку и убить главный реп, потом будет очень весело его восстанавливать.
Если работа идёт через свой форк для последующего залития в основной репозиторий, то IMHO использовать force нормально.

Также GitLab/GitHub/BitBucket позволяют защитить любую ветку в основном репозитории от перезатирания с помощью force, даже если есть админские права, чтобы никто случайно не залил что-то не то, поэтому при грамотном администрировании не вижу проблем.
В принципе, привычка работать одному в своей ветке тоже снимает страх форс-пуша.
В master форсить правда опасно, у нас он, например, закрыт для таких фокусов. А в рабочую ветку — вполне нормально, особенно если над ней корпеет один человек
У меня уже на второй работе есть люди которые вместо сообщения в коммитах указывают ".", или "---", или "***", или ещё что-то такого же стиля. И таких коммитов может быть штук 20 в одном MR. И кратенько в MR написано что «добавлено/исправлено/изменено то-то и то-то».
Как победить не знаю.

Вторая проблема — один коммит на 2000 строк изменений.

Может быть кто-то подскажет как с этим бороться?
Как победить не знаю.
А попробуйте сами рассмотреть несколько коммитов и попробовать составить к ним внятное сообщение вместо "---". Возможно проблема в том, что оно будет или бессмысленное («отрефакторил файлы foo.cpp и bar.cpp», которые и так в этом коммите очевидно присутствуют) или его просто невозможно кратко сформулировать, а развернуто описывать лень.
Да и просто описывать может быть дискомфортно, программист может невербальными категориями руководствоваться в процессе работы, к примеру видит какой-то класс, выглядящий «некрасиво» и давай его рефакторить, плюс ещё парочку зацепит. А ему предлагается потом эту концепцию сконвертировать из многомерной структры, сформированной в нейронной сети мозга в человекочитаемый вид, лишнее напряжение извилин.
Вторая проблема — один коммит на 2000 строк изменений.

  • Если кодогенерация — смиритесь.
  • Если джун стабильно тоннокодит — просто «Decline: Too long», после нескольких объяснений.
  • Если опытный разраб — поинтересоваться «почему так», поговорить с человеком. Вежливо и уважительно поговорить. Должна же быть причина для подобного поведения. Если особых причин нет — попросить изменить подход к коммитам, показать преимущества «продвинутой» методики. Поскольку работа над оформлением может казаться коллеге скучной — польстите ему. Как только коллега оформил PR «нормально» — похвалите его.
  • Если «менеджер не любит, когда время тратит на писульки» — есть резон свалить из команды или смириться.


У меня уже на второй работе есть люди которые вместо сообщения в коммитах указывают ".", или "---", или "***", или ещё что-то такого же стиля. И таких коммитов может быть штук 20 в одном MR. И кратенько в MR написано что «добавлено/исправлено/изменено то-то и то-то».

Если в MR есть вся нужная инфа — можно и забить. Если информации не хватает — поговорить с человеком, поинтересоваться «почему так». Вежливо и уважительно поговорить. Должна же быть причина для подобного поведения…

Попробую применить советы на практике, спасибо!
Может быть кто-то подскажет как с этим бороться?

А вы уверены, что бороться нужно? Если указание "кратенько в MR написано что «добавлено/исправлено/изменено то-то и то-то»." дает исчерпывающую информацию, то зачем писать больше?
А если коммит на 2к строк — это рефакторинг, в котором все изменения по сути одинаковые, то какой смысл дробить их на кучу коммитов?

Не смотря на то, что краткость — сестра таланта, но обычно в MR на 2к строк делается гораздо больше, чем указано в заголовке.
Даже если предположить, что выполнен рефакторинг на такое большое количество строк, то чисто физически ревьювер может не заметить какой-то мелочи при просмотре казалось бы однотипных изменений (и будет баг), поэтому в общем случае, я считаю, что нужно дробить. Так можно хоть по коммитам идти и смотреть кусками что было сделано.
Иногда надо вливать большой MR и надо его ревьювить, потому что иначе никак, но делать регулярно — это бред, как по мне.
поэтому в общем случае, я считаю, что нужно дробить.

Дробить на что? Ну вот давайте воспользуемся примером из статьи — рефакторинг для замены function на =>
Как предлагаете дробить? И зачем? Считаете, такое дробление действительно в каком-то кейсе может кому-то помочь?


потому что иначе никак, но делать регулярно — это бред, как по мне.

Задач, которые требуют по 2к строк, в принципе регулярно возникать не должно. Если возникают — значит, надо делать декомпозицию, а если сделать декомпозицию — это уже будет несколько ПР, которые будут меньше.

commit — это нечто законченное. Делить добавление функции условно на «добавил заглушку» и «сделал реализацию» обычно нет смысла. Но если выполняется условно замена всех вызовов функции A на B, потому что A станет/стала deprecated, то это одним коммитом, пусть и большим. Но если меняется A на B и тут же по ходу происходит переписывание функции в которой производилась замена на более «красивый» вариант, то я бы такое изменение отделил на следующий MR, а не пытался всё в куче пропихнуть.
И кратенько в MR написано что «добавлено/исправлено/изменено то-то и то-то».

Вообще там может быть даже ссылка на тикет, в котором развернуто описана задача. Беда если эта приписка в MR — это единственное место, из которого можно понять, зачем вообще вся эта деятельность была затеяна.
Зафиксировать правила оформления коммитов и не принимать реквесты на ревью до их выполнения?
заставить человека, который сделал этот коммит — переделать коммит на несколько ¯\_(ツ)_/¯
Есть хорошая практика начинать название коммита с номера и названия в JIRA, а потом краткое описание.

Например:
ABC-123 - Add HTTP Basic Authentication to request

- add login/password to config file
- pass login/password to HTTP request


Причем если коммитов много на одну задачу, то в истории сразу видно, как они сгруппированы.
В моей практике внедрялось после первого предложения на скраме — т.к. легко, быстро и не слишком фанатично.
Не лучше ли выделять такие работы в отдельную ветку, и номер тикета в джире выносить в ее название?
Ветка удалится и связь коммита с номером тикета пропадет.
Почему удалится? Вольется в master, и в мержкоммите тикет будет виден. Ну, и да, если у вас ветки удаляются, то чем поможет тикет джиры в названии коммита, который все равно сгибнет вместе с веткой? Или вы прям в мастер коммитите?

В гите нет веток :-) Когда говорят "ветка удаляется" никакие коммиты не удаляются, удаляется лишь "автоперемещаемый именованный указатель на коммит". В Гите самое оптимальное решение проблемы связи коммитов с тикетом выглядит так:


  1. Именуем ветки с id тикета в названии. Например, Jira умеет прямо из тикета создавать ветки с правильными именами.
  2. Добавляем тривиальный commit-msg хук, обеспечивающий, чтобы вначале описания коммита было имя ветки.

Но если у вас не публичный проект, гвоздями прибитый к ГитХабу, то лучше перейти на Mercurial, где этой проблемы нет by design и где есть ветки, как сущности первого класса.

В гите нет веток :-)

Да, есть «именованная голова», что мало меняет дело — если ее удалить, то работать с коммитами становится… не слишком удобно, мягко говоря.
Вероятно, имеется в виду rebase ветки с последующим вливанием, при котором обычно не создаётся автоматического коммита слияния с названием ветки. Тогда удаление ветки действительно приведёт к отсутствию упоминаний о ней.

В gitlab, например, при вливании запросов на слияние всегда создается коммит слияния. Или даже можно делать squash коммитов, но это повлечёт потерю истории изменений.

В gitlab это тоже можно настроить: Settings -> General -> Merge request. Как в self-hosted версии, так и cloud. Если у вас такого пункта нет, то у вас более старая версия.

Тикеты в JIRA не удаляются с мержем ветки, поэтому можно найти историю за несколько лет назад, когда и разработчик то уже уволился. Бывает в ревью проверяю и полноту описания задачи в JIRA.
Это смотря как мержить, если с созданием мерж-коммита, то да, там будет видно какая ветка была вмержена. Но не все так делают, кто-то предпочитает rebase и fast-forward.

Но и с мерж-коммитом не все так просто. Мне удобнее именовать ветку так чтобы мне было понятно, над чем я в ней работаю. В таком случае ветка может иметь имя WJ-32_refactoring и в таком виде имя ветки попадет в коммит, и скорей всего без дополнительных настроек трекер задач уже не сможет понять, что этот коммит был связан с этой таской.

Бывает ситуация когда в процессе выполнения задачи встречаешься с каким-нибудь мелким багом и просто фиксишь его в рамках задачи отдельным коммитом. Фиксить его вне задачи, может быть не удобным, так как например меняются интерфейсы и после фикса надо будет делать мерж. В итоге получается, что все коммиты относятся к одному тикету, но среди них есть, те, что еще касаются дргуих тикетов.
Конечно лучше. Собственно, так обычно и делаем (git flow).

Но после merge это просто коммит в master, и никакой метки о том, в какой ветке он был сделан изначально, не остается. А иногда надо устроить раскопки, чтобы выяснить, какой коммит внес некую ошибку — и тут полезно знать, к какому тикету относился коммит.
Если номер тикета упомянуть в коммит-месседже, то битбакет его подхватывает. Получается удобно и красиво.
Подскажите, а что такое «MR»?
Merge Request, надо полагать.
MR — Merge Request
PR — Pull Request

MR называется в GitLab, PR в GitHub/BitBucket, но суть абсолютно одинакова.
Сменить место работы )
Несколько дополнений:
  1. Длинные описания коммитов не всегда хорошо. Иногда достаточно «автоматический рефакторинг с ReSharper: переименованы методы, удалены ненужные using, удалены неиспользуемые ссылки».
  2. Длинные описания коммитов (примеры и т.д.) не всегда хорошо. Этой информации место в таск-трекере (Jira, TFS, Redmine, etc). Иногда информацию можно частично задублировать.
  3. Делить один пулл-реквест на коммиты — это не единственный доступный путь. Можно разделить PR на два, в первом пред-рефакторинг, во втором — логика. Может не работать на GitHub (непривычно команде, каждого убеждать нет времени\желания), но работает в локальных командах, если заранее объяснить зачем ты так делаешь.
  4. Сохранять единый стиль — не всегда хорошая идея. Иногда стоит изменить стиль в одном месте «для демонстрации», потом начинать его менять всюду. Если команда в курсе об изменении стиля (точнее, одного аспекта стиля) — все будет норм.
  5. Сообщения на русском — спорно. С одной стороны, русский вариант быстрее и проще. С другой, над английским больше думаешь (по крайней мере, первые годы). Но тут сильно зависит от культуры команды\проекта\департамента\компании.
  6. Тесты. Изменилась функциональность — изменения тестов должны быть в том же коммите. Это критично при стратегии «пушим сразу после коммита» — ибо красные тесты будут ломать билд, что может отвечь DevOps и спровоцировать уведомление в рабочий чат.


За твердый стул для offline-review — отдельное спасибо.
Еще добавлю:
  1. Практика микрокоммитов выручает всегда;
  2. Не использовать мердж со сквошем, иначе смысл в гита пропадает;
  3. Ревью кода нужно делать сразу, в тот же день;
  4. Обязательно нужно связывать git c таск-трекером. И писать в таске как можно больше информации в коментарии. Она пригодиться и вам самим. И это является юридическим документом, заодно;
  5. Связывать похожие таски. Баги редко фиксят до конца;
Ревью кода в тот же день — практика сомнительная. Лично я разрешаю сотрудникам тяпляпать что им угодно и как им угодно дотех пор, пока они не будут готовы дать отмашку на ревью. И вот тогда-то начинаю придираться.
В противном случае человек больше думает о форме, но не о содержании.

Длинные описания коммитов — потом будет мучительно читать историю мержей.


А вот ссылки на тикеты в трекере прямо в первой строке коммитмесседжа — это маст хэв.
Опять же, если трекер умеет подтягивать ссылки из коммитмесседжей, — это очень удобно. (А в Яндексе он умеет). Есть ли такая фича в гитхабе или гитлабе — хз.


Коммитмесседжи "шоб подавился" — все эти "---" — лечатся элементарно.


  • ревьювер не пропустит, как явный саботаж
  • пре-коммит-триггер в гите не пропустит (если настроить, конечно)

Тексты на английском — есть риск получить руинглиш.
Но даже ревьюверы-нейтивы (например, из Гугла, если апстримить что-то в их опенсорсные репозитории) не особо лютуют по этому поводу.


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

Тесты в том же коммите — необязательно.

Интересный вопрос вообще. Вот вы практикуете TDD. Red-Green-Refactor. Вопрос — когда делать коммит? После каждого цикла? После Green и после Refactor?

Я и говорю: у гита не хватает ещё одного уровня.
Микрокоммиты — это продвинутое undo-redo и синхронизация каталогов на нескольких устройствах.
Пулреквесты — это готовые продукты.
А как отмечать этапы разработки "сломал — починил — заполировал — и так семь раз"?


Ну, можно в коммитмесседже писать: "сломал", "починил", "заполировал".
Имхо, так проще всего. (И билдсервер научить: что если в коммитмесседже написано "сломал", то не надо паниковать). (Но я такого на яндексовских билдсерверах не встречал, просто теоретизирую).

Длинные описания коммитов — потом будет мучительно читать историю мержей.
После вашего комментария полез проверять историю самого пробленмого скрипта в проекте. VS показывает сообщения мелким шрифтом (вроде 6 или 8), так что все помещается. Плюс, часть сообщений «обрезается». Грубо говоря, в истории отображаются первые строки сообщений. Видимо, мой стиль "НомерВетки: заголовок Пропуск строки Описание" удачно наложился на инструментарий.

Тесты в том же коммите — необязательно.
Насколько я понял из статьи, автор выполняет git push сразу после коммита. То есть, «по коммиту в пачке». В таком раскладе разделение изменений кода и тестов ведет к красным билдам на билд-сервере. А это сразу сообщение в общий чат, которое отвлекает лида\девОпса и т.д. Плюс, когда ты по истории просматриваешь изменения тестов — становится сложнее выцепить коммит с соответствующим изменением функциональности (если сквоша не было). Nickolaim, ваш конфиг чтобы тесты были в той же пачке коммитов, которую автор отсылает на сервер (git push) или после которой он нажимает перезапуск сборки тоже вполне разумен.

А зачем отвлекать лида от проблем в разработчицкой ветке?
Это как-то глупо настроенный билд-сервер, в режиме повышенной паранойи.


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


И вот всё это должно валиться в общую рассылку? Нууу не знааю.

Это как-то глупо настроенный билд-сервер
Вот сейчас обидно было.

У нас билды падают раз в месяц, так что можно и в общий чат поспамить. Плюс, для DevOps все просто: все сообщения в один канал, никаких дополнительных действий. Видимо, слишком разные у нас проекты (по размеру\требованиям).

Или если за время моей работы кто-то внёс в мастер нечто, конфликтующее с моими правками.
А как изменения в мастере могут конфликтовать с вашими правками в Dev-ветке? Или это изменения внесенные в мастер через PR?

Ну у всех стволовые ветки по-разному называются, тут может быть путаница.
Пусть главная ветка разработки — это dev. (или master, или trunk, или ещё что).
Пусть ваша ветка — это TICKET-123


                                  build 2
dev -----------------------------> (*)
 |                                  |
 |                                  |
(*) <- TICKET-456                   |
 |         |                        |      build 1
 +-------- | ---------------------- | ----> (*)
 |         |                        |        |
 |         |           TICKET-123 --+--------+
 +---------+               |
 |                         |
 +-------------------------+
 |
...

Пока вы делали свой ПР, прибежал ваш коллега и успел влить (с ревью, сборками и тестами — всё по-честному) другой ПР.
На билдсервере слияние тогдашнего ствола с вашей веткой — build1 — проходило успешно, а свежего — build2 — сломалось.


Или, для пущей непонятности:


                                  build 2
dev -----------------------------> (*)
 |                                  |
 |                     TICKET-123 --+
 |                      commit 2
(*) <- TICKET-456          |
 |         |               |      build 1
 +-------- | ------------- | ----> (*)
 |         |               |        |
 |         |               |        |
 |         |               +--------+
 +---------+            commit 1
 |                         |
 +-------------------------+
 |
...

Сломалось между двумя вашими коммитами. То ли потому что вы криворукий, то ли потому что конфликт появился.

Хорошо написано, приятно было прочитать статью, спасибо.
Объединить коммиты можно командой git rebase --interactive master. Напомню, что я работаю в фичебранче FEATURE-1, а master — это название основной ветки.

Я обычно делаю сквош не задумываясь через интерактивный rebase ветки на саму себя. Есть ли принципиальная разница?
Перед ребейзом на мастер можно добавить пулл из апстрима. Тогда ты не только сделаешь интерактивный ребейз, но и переложишь свои изменения поверх самой свежей версии основной ветки кода. Чем актуальнее твой код, тем проще его мержить.

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

На самом деле оно только проблем добавляет. Ведь ты взял изменения из мастера и на них сверху свои накладываешь. Сложно понять наверное где это мерж с уже существующим в мастере кодом, а где сама логика задачи. Поэтому я лично тоже за 2 этапа: сначала логика и тесты, а потом мерж с автоматическими тестами и автоматический пуш в мастер всех изменений. Но это конечно ci-way.
Согласен с предыдущим оратором. По-коммитное ревью, дело неблагодарное. И даже в чем-то наверное излишнее. Поскольку ты тратишь свое время на изучение кода которое, возможно, в следующем коммите уже отсутствует. Либо я что-то не так понял.
Мы, что бы избежать ревью 100500 строк кода, бьем задачи на меньшие (в принципе я думаю, мы не единственные так делаем :) ) и тогда ревью получается веселее, да и попутно много решаются какие-то моменты. От постановки задачи, ее оценки и т.п.

Меня больше всего угнетает, что не все понимают необходимость ревью и относятся к этому халатно. Людей конечно не переделать, но какие можно найти слова, что бы коллеги поняли, что это не пустая трата времени и внимания ревьюера? В принципе, вопрос наверное философский :)

Поскольку ты тратишь свое время на изучение кода которое, возможно, в следующем коммите уже отсутствует. Либо я что-то не так понял.

«Приборка» кода с rebase перед пушем недаром же выполняется. Как раз с целью оставить только значимые изменения.

А от меня риторический вопрос, как отучить разрабов пушить закомментированный код?
А от меня риторический вопрос, как отучить разрабов пушить закомментированный код?

Жать decline.
А от меня риторический вопрос, как отучить разрабов пушить закомментированный код?

Ну тыкните на ревью в него и скажите удалить, делов-то.

А от меня риторический вопрос, как отучить разрабов пушить закомментированный код?

Для JavaScript видел такой экзотический способ. Добавляем правило ESlint, которое разрешает писать комментарии только с большой буквы. При комментировании кода в большинстве случаев первая буква будет маленькой. Таким образом, если закомментировать код то не пройдёт линтинг. В связке с husky это не даст закоммитить.

На практике такой способ не использовал. Это правило ESlint мне кажется необоснованным и слишком строгим.

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

Нельзя допускать попадание подобного в коммиты. Для этого есть отдельные ветки. К тому же, закомментированный код может быстро устареть из-за рефакторингов, потеряв ценность, или даже став неработоспособным.

Код должен быть читабельным, а для этого там не должно быть закомментированных кусков.
Должен, должен, должен… это пройдёт через несколько лет :-)
Наоборот. Это начинается через несколько лет. Причём потом все прекрасно осознают, почему код должен быть читабельным, и почему не должно быть исключений. Всегда необходимо заводить объяснения, привязанные к психологии.

Категоричность суждений касательно субъективных понятий (а пресловутая "читабельность" — именно такое) свойственна либо новичкам, либо идиотам. Если вы не новичок, то у меня для вас плохие новости. Я вам привёл пример, когда закомментированный код оказался очень полезным, что не согласуется с вашей картиной мира. У вас есть выбор:


  1. Попытаться убедить меня, что случилось что-то ужасное. Что бесполезно.
  2. Поправить свою картину мира так, чтобы она согласовалась с поступившими новыми фактами..
  3. [Сарказм]
Я вам привёл пример, когда закомментированный код оказался очень полезным, что не согласуется с вашей картиной мира.

Вы хотите сказать, что наличие примера, который демонстрирует, что что-то может оказаться полезным — достаточный повод это что-то не запрещать?

Нож может быть полезным столовым прибором, а может быть орудием убийства. Достаточный ли это повод, чтобы его запрещать?

Хотелось бы всё-таки получить от вас ответ на мой вопрос.

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

А теперь представьте, что я в 16 файлах закомментировал в разных местах строки, решающие Вашу задачу. Вам нужно:
* раскомментировать только правильные строки (а ведь в таком случае я не один там закомментированный код оставлял);
* понять, почему они были закомментированы (может там баг есть архитектурный, который не скоро, но проявится), а человек мог уже давно уволиться;
* заставить всё это в итоге работать.

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

Вас переубедить у меня нет цели, но комментарии другие люди тоже читают, поэтому я всё-таки распишу, что и почему.

Спасибо, что рассказали, через какие костыли можно решить создаваемые вашей идеологией проблемы.


С соломенным чучелом вы можете упражняться сколько хотите, только пожалуйста, не у всех на виду.


Напоследок оставлю вам пример полезного кода в комментариях:


        // Выключили, ибо замедляет загрузку страницы, когда данных много и пользователь ранее домотал до конца.
        // Можно будет вернуть, когда перейдём с ленивого рендера на виртуальный скролл.
        // Подумать о хранении в истории, а не сессии. Возможно нужно какое-то комбинированное хранилище.
        // scroll_top( next? : number ) {
        //  return $mol_state_session.value( `${ this }.scroll_top()` , next ) || 0
        // }
        // scroll_left( next? : number ) {
        //  return $mol_state_session.value( `${ this }.scroll_left()` , next ) || 0
        // }
3 issue с зависимостями, один из которых proposal. В комментарии должна быть ссылка на соответствующий issue, дабы другие не повторили ошибки. Примеры кода должны также быть указаны в issue.

Вам сюда:
habr.com/post/145592
3 issue с зависимостями, один из которых proposal. В комментарии должна быть ссылка на соответствующий issue

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

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

Но так или иначе сам коммит будет содержать номер issue. Тот же `git blame` покажет всю необходимую информацию.

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

А вот большие комментарии к самим функциям — это хорошая практика. И тут лучше использовать системы вроде Doxygen. Впрочем, зависит от языка. В Питоне — своя система, в Яве — своя.
Существует практики самодокументирования кода и декомпозиции. Если потребовалось написать большой комментарий внутри функции, — значит что-то требуется разнести по более мелким функциям.

Это понятно. Речь о том, что если комментарий все-таки потребовался — он не должен требовать куда-то там идти и разбираться, что к чему, и о чем.

Ну в примере выше совсем другое.
1. Предложение по рефакторингу кода, которые может понадобиться. Как люди вообще узнают, что после рефакторинга потребуется изменить кусок кода рядом с комментарием?
Подумать о хранении в истории, а не сессии. Возможно нужно какое-то комбинированное хранилище.

Это отдельный issue, причём proposal. Комментарий может лежать годами и никто не узнает, что там что-то может понадобиться переделать.

2. Когда будет реализован виртуальный скрол, люди должны будут догадаться изменить тут кусок кода о существовании которого они могут даже не знать.
Можно будет вернуть, когда перейдём с ленивого рендера на виртуальный скролл.

Мало того, значит, уже есть issue на виртуальный скролл. Два isuue можно просто связать.

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

Этот issue таже можно связать с остальными. В результате остаётся только это в коде:
Выключили, ибо замедляет загрузку страницы, когда данных много и пользователь ранее домотал до конца.

Раз уж вы дое… хали до конкретного кода, давайте разберём, что тут происходит.


Это отдельный issue, причём proposal.

Пропозал по улучшению отсутствующей функциональности? Часто вы такой ерундой маетесь? Программист по быстрому накидал хинт тому, кто будет думать над этой фичей в будущем. Скажите ему лучше спасибо, что вообще что-то написал. А ведь мог забить, испугавшись вашей заградительной бюрократии.


Комментарий может лежать годами и никто не узнает, что там что-то может понадобиться переделать.

Пока не понадобится передать, этот комментарий и не нужен никому. Лежит себе, кушать не просит. Аллергики могут легко его свернуть, а то и включить автосворачивание.


Когда будет реализован виртуальный скрол, люди должны будут догадаться изменить тут кусок кода о существовании которого они могут даже не знать.

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


Мало того, значит, уже есть issue на виртуальный скролл.

Нет такой issue. Есть мечта у программиста: "когда-нибудь у меня появится свободное время и я смогу сделать такую интересную штуку, от которой всем станет хорошо".


это уже закрытый issue, в котором можно описать всю ситуацию целиком, чтобы другие программисты могли легко

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

Вы вот когда-нибудь пробовали найти issue не зная её названия и формулировок описания, и даже не зная была ли она вообще?

Да, я писал багрепорты в опенсорс. Там основное правило — проверить, не писал ли кто-либо уже такую же ошибку. Один раз попал на дубликат, но там уже была конкретная причина, до которой я сам не дошёл.
  1. Предложение по рефакторингу кода

Предложениям по рефакторингу в принципе нечего делать в коде, да.

Должна, должны… Вы какую проблему пытаетесь решить этой бюрократией?

Много. И разных. Например:


  • дисциплина(/самодисциплина) — самая главная, тут уже в комментариях упоминали;
  • читабельность — всё и везде должно быть одинакового стиля и все должны придерживаться одинаковых правил написания (сокращает время вникания в чужой код, разработка быстрее);
  • беклог проблем, которые есть в проекте и которые не решены;
  • отчётность — клиент может пожаловаться на баг, тогда можно легко найти этот баг и узнать, например, в какой версии он был исправлен и был ли исправлен вообще.
Вы не назвали ни одной проблемы.
Эх… если действительно интересно, что Вы делаете правильно, а что нет, — посмотрите Code Style разных крупных проектов. Посмотрите, как всё устроено внутри крупных и успешных компаний, с какими проблемами они сталкивались и как их решали. Я, например, так и делаю.

Я все проблемы пережил на своём опыте. И только когда своей головой дошёл до того, как правильно делать, начал изучать, что у других компаний. И оказалось, что мои позиции полностью совпадают с принятыми практиками. Только теперь я понимаю, почему у них такие практики приняты.
эх, вместо аргументов бравирование диаметром яиц.
Мне интересно вести двусторонние аргументированные дискуссии, но при переходе на оскорбления, провокации, отсутствие аргументирования, аргументирование «бравированием диаметром яиц» я предпочитаю дискуссию заканчивать, ибо она не имеет смысла. Видимо намёк из предыдущего сообщения был недостаточно понятен, извините.

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


  1. Выписать решаемые проблемы.
  2. Выписать все необходимые условия решения каждой из них.
  3. Если предлагаемое решение не входит в необходимые условия, значит это не решение.
  4. Выписать все создаваемые всеми необходимыми условиями проблемы.
  5. Сравнить создаваемые проблемы с решаемой.
Давайте так. Внутренние данные компании я здесь не могу разглашать даже, если я их написал. Но я всё равно когда-нибудь опубликую статью (или цикл) по части разработки на подобные темы, будет это не раньше чем через примерно год. Диаграммы и данные подтверждённые источниками — это очень не быстро делается. Мне ещё две книги требуется прочесть до этого. Тогда, если изъявите желание, подискутируем.

Но, на будущее, просьба, не переходить на личности, не провоцировать и аргументировать. Из спора на эмоциях ни одна из двух сторон не получит никакой пользы. А мы всё-таки здесь для взаимной выгоды, а не зря тратить время. Агрессия вызывает лишь упрямство. Правило хорошей дискуссии — если почувствовали норадреналин, либо требуется сделать паузу и подумать, либо вообще прекращать дискуссию.
Ок, подождём пока вы придумаете какую-нибудь проблему. Пока что кроме проблемы «мне так не нравится» от человека прочитавшего много чего и написавшего что-то там мы не услышали.
Такой код можно переписать компактнее, используя стрелочные функции:
const sum = arr => arr.reduce((res, i) => res + i);


В чем смысл такой сокращенной записи, тем более через константу?
Стрелочные функции следует использовать только там где это оправдано: в передачи функции в качестве параметра или когда необходимо сохранить контекст.
Ваш вариант невыразителен и не несет в себе никакой ценности, кроме использования модного синтаксиса.

IMO, вот так будет правильнее:
export function sum(arr) {
    return arr.reduce((res, i) => res + i, 0);
}
В конце обязательно благодарю ревьювера. Он потратил своё время на то, чтобы вникнуть в мой код и сделать его лучше. Разве это не заслуживает приятных слов?

Попахивает излишеством. Разве это не его обязанность — ревьюить код? Назначили ревьюером — пыхти и ревьюй. За это люди, собственно, и получают зарплату.
Ревью кода можно сделать нормально, а можно на отгребись. При этом ревью как раз тот случай, когда на отгребись сделать легко, просто и приятно — случись что виноват автор кода, а не ревьюер. Так что да, сказать «спасибо» нифига не лишне.
Не везде. На моей прошлой работе ответственность лежала и на том кто сделал коммит, и на том кто проводил ревью. Так что за косяки спрашивали с обоих.
Это перекладывание с больной головы на здоровую, в норме на ревью не проверяется даже работоспособность кода или то, что код реализует то, что он должен — это зона ответственности программиста и QA. Ревьюер смотрит только сам код на предмет наличия говнокода, а это понятие растяжимое. В общем странно у вас было на прошлой работе…
Тут вопрос как подходить к ревью кода. У нас было, что есть разработчик и «куратор», он проводит ревью кода и он отвечает за качество проведения ревью. Т.е. ревью было обязанностью, а не актом доброй воли. И чтобы не халявили на ревью, в случае косяка могли и с ревьюера спросить.

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

А какой смысл халявить на ревью, самому же потом придется работать с говнокодом, если говнокод этот пропустишь?

Никому нет смысла халявить :) все равно придется разгребать и плохой код, и баги. Но не все и не всегда это понимают.
В описании коммитов стараюсь избавляться от всех неоднозначностей. При помощи ASCII-графики описываю тестовый пример.

Мне кажется, что это полезнее делать в виде теста.
Использовать ли ASCII графику или текст — это вторично; главное, чтобы понятно было.
Мне другое интересно: как заставить разработчика тратить чуть больше времени на описание коммитов, нежели на формальное «Update blablabla». По моему опыту сопротивление этой практике со стороны команды очень сильное.
Я именно про тест. Вот написали Вы метод, оставили комментарий касательного того, что метод делает, описали контракт метода и все. Если нужны примеры работы метода — смотрите тесты.
По моему опыту сопротивление этой практике со стороны команды очень сильное.

Это значит, что коммиты маленькие, а с-но то описывать там и нечего.

Это значит, что коммиты маленькие...

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

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

Они и без сообщения откроют код и узнают.

Чем набирать git show на каждый маленький коммит вместо понятного сообщения? Ну-ну.


Я тут быстренько накидал пример качественно написанного кода на bash с исправлением ошибки в виде diff и, например, сделал коммит "Исправлена ошибка":


--- 1   2018-09-15 23:54:44.000000000 +0300
+++ 2   2018-09-15 23:54:09.000000000 +0300
@@ -1 +1 @@
-mapfile -t source_files <<(find "$source_dir" -name *.c)
+mapfile -t source_files < <(find "$source_dir" -name *.c)

Во сколько раз больше времени Вы потратите на понимание этого исправления (правильно ли исправлено)? И насколько быстро Вы сможете найти этот коммит через неделю, когда Ваш коллега скажет Вам, что проблема с предварительной обработкой файлов исправлена им пару недель назад, чтобы убедиться, не та ли это регрессия, которая вдруг возникла? Ведь коммит мог быть и таким:


--- 2   2018-09-15 23:54:09.000000000 +0300
+++ 1   2018-09-15 23:54:44.000000000 +0300
@@ -1 +1 @@
-mapfile -t source_files < <(find "$source_dir" -name *.c)
+mapfile -t source_files <<(find "$source_dir" -name *.c)

А ведь коммит мог быть "Исправлена ошибка предварительной обработки файлов с исходным кодом". И через пустую строчку даже могли бы быть расписаны симптомы бага.

и, например, сделал коммит "Исправлена ошибка":

А зачем, можно же номер бага в трекере указать было?

А, то есть, в баг трекере описывать мелкое изменение нужно, а коммит описать — сложно? Да даже если в баг трекер завезли, то и номер issue должен быть, и краткое описание изменений. `git log` должен понятным образом описывать изменений. Но работает это, когда все члены команды придерживаются общих правил.
А, то есть, в баг трекере описывать мелкое изменение нужно, а коммит описать — сложно?

То есть, описывать баги в трекере — это использование инструмента по назначению, использовать вместо трекера костыли в виде описания коммита — использование инструмента не по назначению.
Используйте инструменты по назначению и вам не придется использовать костыли.


то и номер issue должен быть, и краткое описание изменений.

Что вообще такое "краткое описание изменений"?
"в строке х заменил код y на z"? а зачем это писать, если это само содержимое коммита?

а зачем это писать, если это само содержимое коммита?

Чтобы не приходилось смотреть дифф каждого коммита, пытаясь понять что и как он изменяет.


c6060c7 $mol_view_tree: don't force default value
9a3de41 $mol_app_tree - online view.tree compiler
6b3a28b $mol_textarea: tab for indentation, scroll styled
Чтобы не приходилось смотреть дифф каждого коммита, пытаясь понять что и как он изменяет.

Но ведь смотреть все равно надо будет, иначе как ревью делать?
На самом-то деле все эти описания делают только хуже, т.к. вызывают некоторое чувство уверенности, что в коммите "tab for indentation, scroll styled" будет только "tab for indentation, scroll styled", что, конечно, не так.

Системы контроля версий не только и не столько для ревью используются.

Системы контроля версий не только и не столько для ревью используются.

Конечно, еще для поиска, например. И поиск сильно упрощается если коммиты достаточно крупные (чем больше коммитов, тем дольше их перебирать, с-но сложнее поиск) и с минимальным количеством информации в описании (описание же надо прочесть, чем оно длиннее — тем больше времени тратится)


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

Ревью проверяют не сразу все команды. А вот полюбопытсовать, что было влито в master-ветку могут все.
  • поправка: "не сразу все члены команды"
Изменение делается не просто так. Оно имеет определённую цель, вот эта цель и описывается. В коммитах нежелательно упоминать ни файл, ни строку. А именно — суть изменения. Например, «исправил такую-то ошибку». Или «Не использовать устаревшую функцию» такую-то.

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

Но, разумеется, если исправляется количество пробелов в отступе согласно внутренним стандартом — можно написать «стилистические правки». Если код слегка порефакторился для упрощения его понимания, — «повышение читабельности».
Изменение делается не просто так. Оно имеет определённую цель, вот эта цель и описывается.

Ну так вопрос — зачем описывать это все в коммите, если можно описать в таск-трекере, где это гораздо удобнее, а в коммите оставить ссылку?

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

Погодите, трекер и так открыт, ведь вы в первую очередь в нем ищите.


Краткое, но внятное сообщение в коммите гораздо удобнее в этом плане.

Только не забывайте, что вам придется прочитать сотни таких сообщений, прежде чем вы найдете нужное. Сколько на это времени уйдет?

Погодите, трекер и так открыт, ведь вы в первую очередь в нем ищите.

В трекере открыта своя задача. Я же имел в виду ситуацию, когда нужно разобраться с уже существующим куском кода (git blame/svn annotate/etc).

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

Только не забывайте, что вам придется прочитать сотни таких сообщений, прежде чем вы найдете нужное. Сколько на это времени уйдет?

Какие сотни, апчомвы? Commit id, трогавший интересующие нас строки, известен (см. выше). Остальные мессаджи можно не читать (git log/svn log прекрасно работают с указанием конкретных ревизий).
Какие сотни, апчомвы? Commit id, трогавший
интересующие нас строки, известен (см. выше).

Мы, видимо, о разных кейзах. Я говорю про поиск коммита для известной задачи, а вы зачем-то ищите задачу под известное изменение кода. Зачем и когда нужно первое — мне ясно. А вот второе? В каких случаях и для чего может понадобиться искать коммит для кода перед глазами? Я, если честно, даже выдумать такую ситуацию не могу.

В каких случаях и для чего может понадобиться искать коммит для кода перед глазами? Я, если честно, даже выдумать такую ситуацию не могу.

Счастливый вы человек. Вот без подколок — искренне завидую.
А ситуацию даже выдумывать не надо, она у меня каждый рабочий день перед глазами. Огромный проект, куча клиентов со своими особыми требованиями, несколько раз менялись требования, иногда выплывали баги, которые надо было фиксить вотпрямщас. Есть исходники по мегабайту размером (и это не шутка и не автогенерация, это код ручной выделки). Очень многие вещи можно сделать по-разному — и в одних условиях это будет правильно, в других станет ошибкой.

В общем, часто по коду непонятно, почему он написан именно так и при каких условиях он должен работать. Помогает аннотация кода: если код — древний, то в нем, скорее всего, нет ошибки (но и лезть туда не стоит — грабли могут выскочить в самых неожиданных местах). Если код трогали недавно, то смотрим внимательнее, кто и зачем это делал.
Понимаю, что наш проект — далеко не эталон с точки зрения методов разработки, но тут уж какой есть.
Git? Работаете с подмодулями? Или поддеревьями? Или гибридно? На подмодули — своя система скриптов или какую-то готовую взяли, если используете?

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

UFO just landed and posted this here

Мелкие коммиты сами по себе хороши по крайней мере в двух случаях: 1) Когда приходится пользоваться annotate. Непонятно, зачем нужен определённый сниппет — аннотируешь, смотришь оригинальный коммит — и там либо вся логика понятна, либо есть ссылка на issue, где описана проблема, которую он решает. Ну, или не решает (если это был тупо рефакторный коммит форматирования/переименования), и тогда сниппет можно смело менять. 2) Когда в разработке есть сравнительно долгоживущие ветки. Тогда чем меньше и логичнее отдельные коммиты — тем легче другим мержить их себе в ветки, если при этом возникают конфликты. Особенно, когда эти конфликты не текстовые, а логические (т.е. когда в vcs всё смержилось без сучков и запинок, а проект при этом "не взлетает", потому что, например, логика/архитектура в небольших деталях поменялась.
Но вот ребейзить ветку поверх свежего мастера — это многогранное решение. В смысле, нельзя сказать, что однозначно хорошо или однозначно плохо.
Про плюсы уже сказали. А минус — никто скорее всего не будет после ребейза тестировать КАЖДЫЙ из коммитов в потоке; посмотрят только, чтобы HEAD собирался/работал ок. Вроде как это, с одной стороны, никому не нужно. Но как только наступит момент поиска баги бисектом — тут-то и вскроется, что некоторые коммиты, оказывается, вообще не собираются...

В мастер надо делать мержкоммиты, по одному на пуллреквест.
Тогда бисект покажет, на каком именно пуллреквесте что-то сломалось.


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

Я часами пытался разобраться в коде, по сотне раз скроллил от функции к тесту и обратно.

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


Писал десятки бесполезных комментариев о пропущенной точке с запятой. Всё это жутко меня раздражало.

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


Я избавляю ревьювера от необходимости писать десятки бесполезных комментариев о пропущенном пробеле. Всё внимание будет сконцентрировано на действительно важных вещах.

Если вы сами признаёте, что это не важные проблемы, то зачем о них вообще писать? Ну нет пробела и фиг с ним, кто-нибудь потом добавит.


При помощи ASCII-графики описываю тестовый пример. Если решению предшествовало обсуждение, где мы рисовали схемы на доске или в блокноте, то прикладываю к описанию ссылку на фотографию.

То есть вы пишете там документацию по реализованной вами функциональности. Как её искать потом? Через git blame? Или дублируете в readme.md? Тогда что она забыла в описании изменений? Вот через год встречу я в каком-то файле _.map и задамся резонным вопросом "а чем [].map не угодил?" — предлагаете ковырять всю историю за год?


Рецепт полезного код-ревью от разработчика из Яндекса

Как вот этот вот бред про документацию в описании коммита прошёл у вас ревью? :-)


Мы будем делать это тремя командами. Я выполняю их столько раз, сколько файлов изменил.

Вот вам время-то девать некуда. Может освоите наконец работу со "своим любимым WebStorm"? Он вам и изменения покажет и синтаксис подсветит и частично застейджить файл позволит и всё это в пару кликов, без изнасилования клавиатуры.


этот линтер работает "из коробки". Я вижу и исправляю проблему сразу, как только написал код, а не оставляю эту работу на потом.

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


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

От того, что где-то будет использована более лаконичная конктрукция, чем в остальном коде, сложность её понимания не возрастёт. Собственно, потому она тут и была использована, чтобы упростить понимание.


Настало время объединить коммиты пулреквеста в один

Зачем? Мерж-коммит так и так будет один.

UFO just landed and posted this here
завтра фигурные скобки после if в однострочном блоке не поставили

А зачем они там?


там переменные назвали a и b

Это вы тоже считаете не существенным? Я вот не считаю, если это не вычисление дискриминанта.


Через некоторое время в проекте творится бардак, читать код неприятно и тяжело.

Так настройте автоформатирование перед коммитом.

UFO just landed and posted this here
Они обязательны, если язык это требует.

Это что за язык такой требующий фигурные скобки в однострочниках?


О чем и пишется в ревью. Пусть настроит автоформатирование перед коммитом.

Вы не поняли, настройте это системно. Чтобы при разворачивании рабочего окружения автоматически устанавливались хуки и настройки редактора.
husky, editorconfig, tsconfig, directory based idea config — вот это вот всё.

завтра фигурные скобки после if в однострочном блоке не поставили

Вы так говорите, словно это что-то плохое…

Через некоторое время в проекте творится бардак, читать код неприятно и тяжело.

Для этого есть инструменты автоформатирования же. Та же Idea умеет, при чем может дае перед коммитом переформатировать автоматически.
Вы так говорите, словно [не поставить фигурные скобки после if в однострочном блоке] это что-то плохое…

Да, это ужасно. Если язык позволяет поставить фигурные скобки после if их надо ставить всегда. Если не поставить, то кто-нибудь обязательно напишет туда ещё одну строку с соответствующим выравниваем и будет думать, то она выполняется только когда срабатывает if.


Кроме того, если надо залогать что-то для отладки, когда срабатывает if а потом убрать логи — то радости добавлять скобки, а потом удалять — немного.

Да, это ужасно. Если язык позволяет поставить фигурные скобки после if их надо ставить всегда. Если не поставить, то кто-нибудь обязательно напишет туда ещё одну строку с соответствующим выравниваем и будет думать, то она выполняется только когда срабатывает if.

Секундочку! Если кто-то видит, что в if нет фигурных скобок и что-то туда дописывает с ожиданием того, что работать будет, как блок, то это не проблема однострочного if, это проблема компетентности того, кто вставляет строку. Во-вторых категоричное «всегда» опровергается практикой. К примеру такой код:
public void doSomething(String param) {
        if (param == null) throw new RuntimeException("...");
        if (param == "") throw new RuntimeException("...");
        if (param.equalsIgnoreCase("error")) throw new RuntimeException("...");

        doSomething(param, DateTime.now());
    }

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

Кроме того, если надо залогать что-то для отладки, когда срабатывает if а потом убрать логи — то радости добавлять скобки, а потом удалять — немного.

Если вам не хватает дебаггера, и вы вставляете логгер для отладки, то имеет смысл оставить этот логгер в этом месте навечно, просто настроить его на уровень debug. В остальном следует задаться вопросом, что происходит в коде, если у вас возникают подобные желания там, где работает однострочник. Может быть логгировать стоит не рядом с ним, а внутри него?
Секундочку! Если кто-то видит, что в if нет фигурных скобок и что-то туда дописывает с ожиданием того, что работать будет, как блок, то это не проблема однострочного if, это проблема компетентности того, кто вставляет строку.

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


В общем-то ведь иногда можно и вовсе писать код в одну строку. А если у кого-то возникают проблемы с чтением/редактированием такого кода — ну это просто человек некомпетентен. Разве не так?

Если кто-то видит, что в if нет фигурных скобок и что-то туда дописывает с ожиданием того, что работать будет, как блок, то это не проблема однострочного if, это проблема компетентности того, кто вставляет строку.

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


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

Да нет, вот так выглядит лучше и читается намного лучше. Тоже практикой проверено.


public void doSomething(String param) {
    if (param == null) {
        throw new RuntimeException("...");
    } else if (param == "") {
        throw new RuntimeException("...");
    } else if (param.equalsIgnoreCase("error")) {
        throw new RuntimeException("...");
    }
    doSomething(param, DateTime.now());
}

Если вам не хватает дебаггера, и вы вставляете логгер для отладки, то имеет смысл оставить этот логгер в этом месте навечно, просто настроить его на уровень debug

Когда как. Бывает, что имеет смысл оставить его навечно, бывает, что не имеет.

Да нет, вот так выглядит лучше и читается намного лучше. Тоже практикой проверено.

Вы это серьезно? О_о Лучше читается? Лучше выглядит??? Ладно, проехали… *ушел переосмыслять жизнь*
Вы это серьезно?

Серьёзно


Лучше читается?

Да, условие на одной строке, действие на другой. Читается лучше.


Лучше выглядит???

Ну это конечно субъективно, но я подозреваю, что лучше выглядит потому, что читается лучше. А у вас критерий красоты похоже — количество строк. Хотя, конечно в чужое сознание проникнуть сложно ))


Ладно, проехали… ушел переосмыслять жизнь

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

Тоже не понимаю. На JS миллиарды строк кода написаны так:

if (a == b) doSomething(a);


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

Также ему неплохо бы знать, что надо писать не a == b, а a === b )) .


Вбахать туда что-то после точки с запятой может новичок без знания языка вообще

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


но это просто надо знать, это относится к синтаксису языка

Когда части синтаксиса языка являются источником ошибок, лучше ими не пользоваться. Например, не надо использовать в JS for… in…. Вообще. Не смотря на то, что иногда эта конструкция работает нормально и просто надо знать, когда она работает нормально, а когда нет.


Огораживать скобками такие «возможные ошибки» — просто потеря времени и сил.

В стандартах кодирования нередко отражено обратное.

== и === в JS имеют разный функционал, нужно знать и то, и то, и использовать по назначению
== и === в JS имеют разный функционал, нужно знать и то, и то, и использовать по назначению

Смысл у этих операторов действительно разный, знать разницу действительно неплохо и зачастую те, кто эту разницу понимает, == не используют. Или используют в случаях, когда без этого не обойтись. Я таких случаев не припомню.


Но я рад, что по остальным пунктам у нас разногласий не возникло ))

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

Вы, наверное хотели сказать можно, а не надо, да? Если язык позволяет использовать оператор равенства с приведением типов, то можно его использовать?


Либо уже переходить на TS, когда хочется статических типов

=== не про статическую типизацию, он про строгую. В TS точно так же есть == и === с теми же проблемами. Но любители === и любители статической типизации и вправду зачастую лучшие друзья )).


В остальном — как быть, если TS еще не сделали, а баги, связанные с == уже изрядно замучали? По-моему, вывод очевиден.

Этот факт отражён в корпоративных стандартах компаний, которые поняли это на собственном опыте.

В стандартах кодирования нередко отражено обратное.

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

Временами, наверное можно, но в основном лютого шлака там нет. В основном там рекомендации, выработанные годами практики.


что лучше на них не ссылаться.

А на что лучше ссылаться?


Вообще апелляция к авторитетам штука скользкая,

Ну апелляция к авторитету немного лучше, чем апелляция к собственному мнению.


а «стандарты кодирования компании» на такой авторитет не тянут даже близко.

Гугл для вас не авторитет?

> Гугл для вас не авторитет?

Гугл просто раскрученная большая и успешная компания. Мне, например, не нравится гугловский стандарт кодирования для C++. Он имеет странные ограничения и не особо «читаем». Возможно для самого гугла он подходит хорошо. Так что то, что подходит гуглу подходит именно гуглу.
Аналогично с Яндексом, Майкрософт, JetBrains, Касперский, DINS, Facebook, Whatsapp и прочими известными успешными компаниями. Они что-то используют — отлично, можно на это посмотреть, но не стоит слепо это тут же использовать у себя. Выбранный продукт может подходить конкрено для их ситуации, он может быть доработан ими для себя. В конце концов, в этих компаниях работают обычные люди которые тоже могут ошибаться. И кто-то что-то протащил по ошибке и теперь они этим пользуются, так как заменить будет дороже.
Ещё у Гугла плохо продуманные протокольные буферы. Сам протокол. Он мог бы выигрывать по сравнению с плоскими буферами, но типы полей не дают абсолютно ничего. Одно дело, что б указывался тип кодирования поля (varint, signed varint, fixed64, binary), что добавляло бы гибкости в будущей поддержке инфраструктуры, но не тип поля. А поменять ничего не могут из-за обратной совместимости (даже в 3-х).

Когда я разбирался с протокольными буферами у меня упал авторитет Гугла в глазах. А реализация генератора для С++ вообще не понравилась, — раздутый результат, с большим количеством выделений памяти.
Огораживать скобками такие «возможные ошибки» — просто потеря времени и сил.

Это как раз экономия времени и сил. Никаких затрат в том, чтобы сразу поставить скобки — нет, зато ожидание пользы — ненулевое (даже если вероятность ошибки — крайне мала).

public void doSomething(String param) {
        if (param == null) { throw new RuntimeException("..."); }
        if (param == "") { throw new RuntimeException("..."); }
        if (param.equalsIgnoreCase("error")) { throw new RuntimeException("..."); }

        doSomething(param, DateTime.now());
    }

Пример практически не отличается от вашего. Скобки стоят.
Да, так гораздо лучше, но «проблему» при которой
кто-нибудь обязательно напишет туда ещё одну строку с соответствующим выравниваем и будет думать, то она выполняется только когда срабатывает if

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

1. Не для всех. «Разумные люди» — категория слишком размытая.
2. Не станут. Ибо дело привычки.

Что касается «проблемы компетентности того, кто вставляет строку» после if без фигурных скобок — я вас уверяю, такие ляпы допускают все разработчики независимо от уровня компетенции. Конечно, чем человек опытнее, тем меньше вероятность, но даже супер-пупер гуру не застрахованы от таких ошибок.
Полностью согласен с Вами касательно оформления кода для описываемого случая с ошибками.
Но, по моему мнению, однородные операции лучше в if else оборачивать, чтобы видно было, что это какай-то единый оператор выбора.
Т.е. мне кажется правильным
    if (param == null) throw new RuntimeException("...");
    if (param == "") throw new RuntimeException("...");
    if (param.equalsIgnoreCase("error")) throw new RuntimeException("...");

    doSomething(param, DateTime.now());

Но, так же будет правильным
    if (param == 1) {
        do1();
    } else if (param == 2) {
        do2();
    } else {
        doDef();
    }
У примеров семантика разная. Первый пример станет делать избыточные проверки, если вместо бросания исключений там будут, скажем, обычные вызовы функций. Поэтому в общем случае каскад if()… else if() предпочтительнее.

ЗЫ: А во втором примере прямо-таки напрашивается switch/case. :)
Ну вот о том и речь, что на разные случай разный синтаксис
Раньше писал много кода и делал один коммит, а сейчас взял за правило: одна задача — один коммит.
Из статьи вынес ещё пару полезных вещей для себя на будущее.
В принципе, здраво, но не соглашусь про offline. Наоборот, быстрое совместное обсуждение кода разработчика позволяет максимально точно понять, что он хотел и почему сделал именно так. А когда смотришь в код, непонятный сразу, то можно многое додумать, в том числе и лишнее.
И ещё пункт насчёт названия коммитов: мы просто называем следующим образом — JiraTaskName: commit summary. Например, ABC-1001: Сделан вывод ошибки при вводе неправильных данных. Это достаточно удобно для дальнейшей работы, у нас GitLab, ссылки на таски в джиру проставляются автоматически.
А почему не используете встроенную в Gitlab систему issue? Я Jira видел только по демкам, поэтому интересно, в чём основные преимущества.

У нас тоже используется Jira и Gitlab. Как мне кажется, Jira больше про управление проектами, а Gitliab про управление репозиториями. Соответственно, issue в Gitlab привязана к репозиторию, а в Jire к проекту. Jira более удобна для управления процессом, так как заинтересованным лицам не нужно думать в терминах репозиториев и вообще знать, что они существуют.

Меня больше интересует, что она даёт разработчикам? Там есть какая-либо система распределения задач по программистам, если каждой задаче прописано время и проставлены зависимости? Своего рода распараллеливание с минимальными запланированными сроками исполнения. И вообще, там можно выстраивать зависимости, как сделано в RedMine?
Да, там все это есть. И все это настраивается (сам я настройкой не занимался, этим занимались на одном месте специальный отдел, который еще и допиливал jira добавляя в нее специфичный функционал, а на текущем этим занимаются DevOps и QA). Много разных плагинов — есть платные, есть бесплатные.
В отличие от RedMine, Jira это только система управления проектами, там не будет вики для проекта и репозитория для него. В качестве wiki предлагается использовать Confluence, а отображение коммитов связанных с issue достигается интеграцией с Bitbucket. Вроде, можно настроить, чтобы в коммите можно было указать время затраченное на задачу и оно учется в Jira, можно, чтобы issue при ее упоминании переходила в определенное состояние, но мы этим не пользовались.
Можно ли jira так же интегрировать с Gitlab — думаю, что да, но я этим не занимался и это только мое предположение.
С Gitlab можно. В Gitlab wiki есть. Не очень удобный, но всегда можно WikiMedia поставить и интегрировать. Там по сути просто ссылка в пункте меню меняется и всё.

А в плане задач меня интересовала следующая схема. Я создаю множество задач, каждой выставляю оценочное время выполнения. Проставляю между ними зависимости. Затем делаю что-то вроде milestone'а, указывая, какие программисты в нём должны участвовать. Система в итоге должна построить диаграмму Ганта для скорейшего достижения milestone'а. В идеале ещё было бы хорошо, чтобы каждой задаче можно было проставлять оценку сложности для корректного распределения между юниорами, мидлами и сеньорами.

Пока по описаниям не нашёл ни одной такой системы.

Такого "из коробки" Jira не умеет. Только если писать плагины для нее самому. Зависимости задач есть, оценка сложности — в Jira можно добавлять собственные поля. На моей прошлой работе было сделано такое: можно было выбрать разработчиков участвующих в проекте и распределить между ними задачи (вручную), при этом показывалась загрузка разработчика на указанный период по другим проектам, но это наш отдел сопровождения Jira ее модернизировал.


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


Я тоже подобного не встречал, но выглядит как полезная и востребованная вещь. Можно попробовать поискать в плагинах для Jira, по этой теме попалась пара плагинов: Assign & Escalate for Jira и Issue Alternative Assignee. Но это поверхностный поиск, может можно копнуть глубже и найдется, что-то более подходящее.

Пока по описаниям не нашёл ни одной такой системы.

Их и не существует, потому что оптимальную диаграмму ганта нельзя построить по оценочному времени. Только по четко заданному, которое гарантированно не изменится.

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

И, с-но, диаграмма ганта не может быть построена. По-этому описываемых вами систем не существует и не может существовать.


Но ничего не мешает её перестраивать по ходу решения задач.

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

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

Я ещё только вникаю в Agile. Но, если я правильно понимаю терминологию, это автоматизация Scrum-мастера.
Но, если я правильно понимаю терминологию, это автоматизация Scrum-мастера.

Автоматизацией она является в том случае, если дает валидный результат.
Вы же не назовете "автоматизацией уборщика" систему из набрасываемых на вентилятор кусков говна? Нет. Почему? Потому что в итоге работы этой системы чище не становится.

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

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

Дает, но только при условии, что сроки однозначно заданы и не поменяются.
Если они могут поменяться — то уже не дает.
Можно с тем же результатом просто рандомно раскидать задачи.

Ручной труд никто не отменял. Кому хочется, может тратить на него своё время. Одна из задач программистов (в том числе, кстати, и по ГОСТам) — автоматизация. Далее всё зависит от того, сколько она сэкономит средств и сколько будет стоить автоматизация. И тут, разумеется, выгоднее покупать сторонние решения, специально заточенные под задачу, если они существуют.
Только недавно узнала про прием с объединением нескольких коммитов в один с помощью rebase. Это действительно очень удобно.
И в общем — статья действительно хороша. Прям идеальная последовательность действий, которую наконец кто-то взял и четко сформулировал.
Автору большое спасибо!

Рекомендую прочитать книгу ProGit, все оттуда с первого раза, мне было, сложно запомнить, да и не к чему оно, но зато есть понимание куда смотреть если возникает вопрос.

> git status
> git diff comments.js
> git add comments.js

Я выполняю их столько раз, сколько файлов изменил.


А не удобнее ли git add --interactive? Я вот без него жить не могу: очень удобно и ревьювить перед коммитом и разбивать код на микрокоммиты.

Если вы пользуетесь WebStorm, то зачем писать коммит сообщение в Vim? В настройках Intellij IDEs есть настройки Commit Dialog, там можно указать длину subject line, длину body. Получается аналогично тому что в Виме. Плюс есть базовый spell checker.


А команду git log чтобы сформировать описание пулл реквеста когда и как используете? Выполняете команду, а потом вручную копируете текст и затем вставляете в GitHub?

Привычка, которая осталась с тех времён, когда у меня ещё не было WebStorm. Скорее всего, все действия, которые я делаю при коммите, можно выполнить в IDE.

Мне хочется все команды для работы с git вызывать в одном месте. Могу ли я в IDE выполнить ребейз с флагом --interactive или лог с ключами --pretty='%h: %B' --first-parent --no-merges --reverse? Если можно, с удовольствием полностью перееду в IDE.

А команду git log чтобы сформировать описание пулл реквеста когда и как используете? Выполняете команду, а потом вручную копируете текст и затем вставляете в GitHub?

Да, выполняю в консоли и копирую вывод в описание к пулреквесту.
Sign up to leave a comment.