docs(project-repo): Add code practices #19
Conversation
There was a problem hiding this comment.
Thanks for tackling this!
Do not hesitate (maybe you wanted to do that in another PR) to transform from the google docs dumping notes to actual sentence and rephrasing directly what was note takings to actual consummable content!
I have written some comments as part of that (and also, the description of the PR will be the commit message content, so it can be more elaborate).
|
|
||
| - All new code comes with tests. Fixes not coming with tests is rather the exception than the norm. | ||
|
|
||
| - We follow upstream/community formatting conventions for the given technology. For instance. in Go, we tighten it using `gofumt` instead of `gofmt`. In Rust, `rustfmt`. |
There was a problem hiding this comment.
I think those 2 parts are actually separated. Especially the use of gofumt (which is stricter than gofmt, but compatible: what pass gofumt will always pass gofmt. However, not all "gofmted" code will necessarily pass gofumt).
|
|
||
| - We should not have any warning/error when building a project. Any of those needs to be "clear" (with a comment explaining why we are ignoring this warning). | ||
|
|
||
| - Every method have a docstring/annotation explaining what the function does, what it accepts and what it returns. Also, for languages raising exceptions, if any kind of specific exception that this method raises. |
There was a problem hiding this comment.
The second sentence is not complete? :) (could be a copy of my google doc and I was interrupted in the middle of the sentence). I think the idea is to document what exceptions types the method can raise for those languages.
There was a problem hiding this comment.
Btw: for simple private method/function, the docstring requirement (if obvious) can be leverage in some cases IMHO. Like an add method which doesn’t need return any method doesn’t really need a doc comment.
|
|
||
| - Avoid using global variables in modules. Prefer a struct that embeds the state and that can be mock for other package level tests. | ||
|
|
||
| - Commit convention: something I have a less strict opinion about. I think there should always be a description and a short commit title. The commit title can reference the domain of the project this work is acting on. There is some proposal to use Conventional Commits, how do we relate it to non squash commits if we don't follow this? |
There was a problem hiding this comment.
ok, this is definitively a bare copy of my dumped notes on the google doc, shouldn’t we rephrase it first? Also, this evolved (see the PR session our squads gave at Riga + the pull request doc).
|
|
||
| - Info: those are the expected information to the admin looking at high level information for their system `listening on socket <addr>`, `got request <details>`. | ||
|
|
||
| - Debug: low level of detail information, preferably on a per function level, naming the function and incoming arguments. Also, use to differentiate where in the code we are branching in. This is typical "for developer debugging" information level. |
There was a problem hiding this comment.
on formatting: maybe emphase the keywords "Error/Warning/Info…"?
|
|
||
| - Debug: low level of detail information, preferably on a per function level, naming the function and incoming arguments. Also, use to differentiate where in the code we are branching in. This is typical "for developer debugging" information level. | ||
|
|
||
| - Opened for discussion (as in general, a little bit less relevant for the desktop): use of structured logging library. |
There was a problem hiding this comment.
I guess this is a note as a TODO in the end of the doc?
|
|
||
| - Opened for discussion (as in general, a little bit less relevant for the desktop): use of structured logging library. | ||
|
|
||
| - Early return: try to align code on the left as much as possible. The happy path is on the left, the nested part are particular use case, with early bailing out. Basically, the `else` clause is more the exception than the rule. Functions can be shortened in addition to the readability this gives to the reading flow. |
No description provided.