Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[建議] 支援更好的命名及一致的coding style #53

Closed
titaneric opened this issue Feb 27, 2021 · 10 comments · Fixed by #54
Closed

[建議] 支援更好的命名及一致的coding style #53

titaneric opened this issue Feb 27, 2021 · 10 comments · Fixed by #54
Assignees
Labels
enhancement New feature or request

Comments

@titaneric
Copy link
Collaborator

titaneric commented Feb 27, 2021

原本作法的問題 / Solved Problem

  • 目前在命名上有幾個golint會跳警告如下
    • ALL_CAPS命名, Ex: PTT_BRD_POSTMASK
    • JSON, Ex: favJson
    • UI, Ex: PosOfPttPasswdPagerUiType
    • Error命名, Ex: InvalidFavTypeError
    • 不一致的receiver命名
    // 前面用b
    func (b *BoardHeader) BoardID() string { return b.BrdName }
    // 後面用r
    func (r *BoardHeader) MarshalBinary() ([]byte, error)
  • 一些語法可以更好
    • idx += 1 -> idx++
    • 移除unreachable code

實作細節 / Details of Implement
按照go lint建議改,這應該跟 #52 一樣問題。其中ALL_CAPS命名個人覺得有討論空間,畢竟原始的code是C

期程 / Schedule
(希望這個任務大概什麼時候做完,如果會卡到其他地方的功能也請在這邊提出。)

  • 討論時間: 一週
  • 實作時間: 一週
  • 確認時間: 一週

相關文件 / Documents
Follow golint

另外是我已經開始著手更改了,麻煩assign給我,謝謝

@PichuChen
Copy link
Member

ALL_CAPS 的問題我可以很確定應該要改掉他,然後 PosOf 的問題也是要被改掉

@PichuChen
Copy link
Member

receiver 的命名有哪邊有文件介紹相關的規範嗎? 因為我覺得他在同一個 name scope 底下的命名其實問題不大。

@titaneric
Copy link
Collaborator Author

應該是類似golint下的測試
裡面有一個// MATCH /receiver name a should be consistent with previous receiver name b for bar/

@minchao
Copy link
Contributor

minchao commented Mar 2, 2021

@PichuChen 關於 Receiver Names 的命名,在官方 CodeReviewComments 文件裡提到:

Be consistent, too: if you call the receiver "c" in one method, don't call it "cl" in another.

@titaneric
Copy link
Collaborator Author

titaneric commented Mar 2, 2021

這邊我覺得可以跟各位討論ALL_CAPS該怎麼改比較好,例如:PTT_IDLEN中間有縮寫命名的是改成PttIdLen嗎?會不會用IdLen就可以,因為我們有用module區分不同的bbs,PTT也許就可以省略。
另外我不太明白 @PichuChen 說的PosOf 的問題是指哪一個

@minchao
Copy link
Contributor

minchao commented Mar 2, 2021

const PPT_IDLEN 為例,參考官方文件:

PTT 部分,參考 Package names,使用 package 來組織程式碼,我們可以拿掉 PPT 前綴,避免 stutter:

Avoid stutter. Since client code uses the package name as a prefix when referring to the package contents, the names for those contents need not repeat the package name. The HTTP server provided by the http package is called Server, not HTTPServer. Client code refers to this type as http.Server, so there is no ambiguity.

ID 部分,CodeReviewComments 提到:

This rule also applies to "ID" when it is short for "identifier" (which is pretty much all cases when it's not the "id" as in "ego", "superego"), so write "appID" instead of "appId".

LEN 部分,Organizing Go Code (2014 Google I/O talk) 提到:

Global variables should have longer names.

@PichuChen
Copy link
Member

PTT_IDLEN 的問題我認為應該要被修改成 IDLength 或者是他應該要是一個 getter 假如這個值有可能被動態設定的話。 但是必須要有註解讓大家知道他要被參照到 原本的地方

@minchao
Copy link
Contributor

minchao commented Mar 2, 2021

補充,Golint 處理的是 Coding style,所以有些規則可能見仁見智,雖然其文件提到 Google 正在內部使用,但爬一下 Go 原始碼也是可以發現許多反例 XD。且它是一個全有或全無的檢查器,無法忽略任一檢查規則。

@titaneric
Copy link
Collaborator Author

請問@PichuChen 指的是類似PosOfFormosaBBSFileHeaderFilename以及PosOfPTTBoardName這種有bbs各站的命名嗎?這在pttbbs/README.md有提到。

另外golint看起來的確不打算支援ignore某些檢查(golang/lint#186),可以的替代方案是有如golangci-lint。但這些是非官方實作,若要放入CI/CD流程可以再開一個issue討論。

@PichuChen
Copy link
Member

對,原本因為是在同個 package 所以用 PosOf 切開,但是現在已經分成不同套件了,所以不需要切開了。

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
3 participants