關於pull request這檔事

之前寫過關於code review這檔事,那反過來說,關於發pull request這檔事呢?(或者說發patch也適用,以下就姑且都說PR吧,節省篇幅愛地球。)

Automattic的夥伴們都挺愛挑毛病修細節的,PR來回修個幾週才進、打掉重寫,或是要拆細重發等等多不勝數,所以以下也算是個人在敝公司發PR的生存守則:

  1. 單一目的
  2. 測試方法清楚簡單
  3. 迭代大於完美

1. 單一目的

白話一點說,就是一個PR只做一件事,而且要在PR的summary處描述清楚。所謂『只做一件事』有點模糊,畢竟每個人的標準不太一樣,而且切得過細也不見得是好事,失去大方向可能會讓reviewer看得一頭霧水。那怎樣的大小叫剛好?我覺得較通用的原則就是能切出來獨立運作的就切,這樣通常就不會太大。

舉例來說,在開發calypso的時候,如果我們要加一個頁面,而該頁面會與某service API溝通,常見的做法是切成至少:

  • app state一個,也就是redux reducer / selector / action creators
  • API溝通一個,也就是data layer handler
  • UI一個,視UI複雜度可能會拆細成components PRs數個與最後組合成頁面的PR

2. 測試方法清楚簡單

自己寫的程式碼,自己理當要是最清楚怎麼測試的人,而不是要reviewer發揮創意幫你想辦法測試。應當要由自己提供清楚、step by step就能完成基本正確性驗證的測試,這時再由reviewer去發揮創意想沒涵蓋到的地方。基本原則是所有code path都要跑到,當程式碼是JavaScript或PHP這種語言的時候這特別重要,畢竟compiler不會幫你檢查沒跑過的地方是不是根本就是一坨XX。

至於什麼樣叫『簡單』呢?應該能自動化就自動化,不能自動化的部分就要指示清楚明瞭。data flow的部分幾乎都能unit test所以就該寫unit test、React component可以用enzyme或jest snapshots、service APIs的部分可以用來確定各種API response下的行為、實際與service API溝通的部分可以寫個script發真的request並檢查所有response是不是都和預期相符、end to end tests比較麻煩但還是可以做,例如wp-e2e-tests

以上節範例來說,app state和data layer handler的部分都很容易可以unit tests,那就應該要力求一行npm run test-client就結束。UI的部分不見得會寫unit tests,但一定要附上詳細的使用流程,力求讓reviewer無腦地跟著指示就能用、就要知道預期的結果為何。

3. 迭代大於完美

最後,與其等一切都臻於完美再發PR,不如一開動就發,標上work in progress,再逐步迭代。個人經驗上,這在參與開源專案或遠端工作都很有幫助;公開透明、勤於溝通,是這兩者共通的生存必備技能。我過去曾有幾次自己埋首寫了一週,結果一丟出來馬上就被整個翻掉。這不但對自己的士氣是一大打擊,也會拖累整個團隊。而且,這與其說是追求完美,不如說單純只是怕丟臉罷了。

螢幕快照 2018-01-29 下午12.26.52

儘早公開你在做的事,不但讓同組的人可以第一時間參與討論,及早修正方向,也常常可以釣那些閒閒沒事逛PR當小說在看的人進來幫忙。

關於pull request這檔事 有 “ 2 則迴響 ”

發表迴響

在下方填入你的資料或按右方圖示以社群網站登入:

WordPress.com 標誌

您的留言將使用 WordPress.com 帳號。 登出 /  變更 )

Google+ photo

您的留言將使用 Google+ 帳號。 登出 /  變更 )

Twitter picture

您的留言將使用 Twitter 帳號。 登出 /  變更 )

Facebook照片

您的留言將使用 Facebook 帳號。 登出 /  變更 )

w

連結到 %s