PR 리뷰의 한 가지 — 무엇을 안 보고 무엇을 보나
리뷰에서 진짜 봐야 할 것과 버려야 할 것을 나눈 기준.
리뷰에서 버린 것들
오래 리뷰하다 보면 이상한 습관이 생긴다. 들여쓰기, 변수명 casing, 줄 끝 공백. 막상 댓글을 달고 나면 작성자는 아무 생각 없이 고치고, 나는 아무 생각 없이 승인한다. 그 교환이 리뷰라고 착각했던 시절이 있었다.
Linter 가 잡을 수 있는 건 Linter 에 맡겼다. ESLint, Ruff, gofmt. CI 가 빨간불을 켜면 그게 댓글보다 빠르다. 이 결정 하나로 리뷰 댓글 수가 30% 줄었다. 줄어든 댓글은 잡음이었다.
“이 함수 이름이 좀 더 명확하면 어떨까요?” 류의 취향 댓글도 거의 안 단다. 일관된 컨벤션이 없는 상태에서 이름 하나 바꿔봤자 코드베이스는 그대로다. 컨벤션이 있다면 Linter 나 문서가 잡는다. 없다면 그 PR 에서 고칠 게 아니라 따로 결정할 문제다.
실제로 보는 것
세 가지만 본다.
첫째, 경계 조건. 가장 먼저 본다. 입력이 비었을 때, 리스트가 0개일 때, 외부 API 가 500 을 반환할 때. 정상 케이스는 작성자도 이미 생각했다. 리뷰어가 추가로 볼 수 있는 건 엣지다. 최근 서비스에서 items 가 빈 배열이면 items[0] 에서 터지는 버그가 있었는데, 리뷰에서 한 번만 짚었어도 됐다. 그 버그는 배포 다음날 오전 9시에 터졌다.
둘째, 롤백 가능성. 이 변경을 되돌려야 할 때 얼마나 걸리나. DB 마이그레이션이 포함돼 있으면 down migration 이 있는지 본다. feature flag 없이 전체 배포가 되는 변경이면 물어본다. 특히 스키마 변경 + 코드 변경이 한 PR 에 묶여 있으면 분리를 요청한다. 롤백할 때 두 가지를 동시에 되돌리는 건 새벽 2시에 가장 어렵다.
셋째, 조용한 실패. 에러를 삼키는 패턴. try/except: pass, 에러 로그 없이 return None, 고루틴 안에서 에러를 채널에 안 보내는 경우. 이런 코드는 배포 직후엔 아무 일도 없다. 2주 뒤 새벽에 왜 데이터가 없는지 아무도 모른다. 이 패턴 하나만 잡아도 온콜 호출이 눈에 띄게 준다.
나머지, 예를 들어 “이 로직을 함수로 분리하면 어떨까” 류는 nit 으로 달거나 그냥 건너뛴다. 아키텍처 문제라면 PR 이 아니라 설계 단계에서 얘기했어야 한다. PR 은 그 논의가 끝난 결과물이다. 그 자리에서 구조를 바꾸자는 댓글은 작성자 입장에서 가장 피로하다.
한 가지 더. blocking 댓글과 non-blocking 댓글을 명확히 구분해서 달기 시작했다. [BLOCK] 이라고 prefix 를 붙이거나, GitHub 의 Request Changes 는 진짜 blocking 이유가 있을 때만 쓴다. 그 전에는 Comment 와 Approve 를 별 구분 없이 썼다. 작성자는 Comment 가 달리면 어디까지 고쳐야 merge 해도 되는지 몰랐다. 내가 잊고 넘어간 댓글이 2주 뒤 “이거 반영 안 하셨어요?” 로 돌아온 적도 있었다.
다음 한 가지
다음 PR 에서 댓글을 달기 전 — 이걸 Linter 나 테스트가 잡을 수 있나, 한 번만 먼저 따져본다.
🛒 이 글과 어울리는 추천 상품
위 링크는 쿠팡파트너스 활동의 일환이며, 일정액의 수수료를 제공받을 수 있습니다.