-
Notifications
You must be signed in to change notification settings - Fork 14
docs(project-repo): Add code practices #19
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
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,41 @@ | ||
| # Code practices | ||
|
|
||
| - 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`. | ||
|
|
||
| - The code should always be linting-error free. | ||
|
|
||
| - 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. | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
|
|
||
| - Two code copies are OK, if you start having a third one, it's probably time to generalize/factor it out. | ||
|
|
||
| - Always have private interfaces/modules first, and only open them up if you use them. Do not pre-determine that this element should be public because you will use it one day. | ||
|
|
||
| - 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? | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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). |
||
|
|
||
| - Releases are tagged as SEMVER. | ||
|
|
||
| - We should keep version 0.x until we are shipping a given product is GA for the Product Manager. | ||
|
|
||
| - No premature optimization: maintainability > performance. Write your code to be maintainable (low cyclomatic complexity in code, clear variables, commented code to explain non trivial parts). If then, some code becomes critical and you have a suspicion this part is what is triggering a performance issue, don't assume, prove it first. Measure, analyze, fix. | ||
|
|
||
| - Logging: We only allow some restricted levels of logging: Error/Warning/Info/Debug. | ||
|
|
||
| - Default logging level is Warning. | ||
|
|
||
| - Errors are unrecoverable issues, theoretically making the program stop or abort critical tasks that could not be retried. | ||
|
|
||
| - Warning are important information to the administrator when a recoverable task or an unimportant task fails. The normal running execution of a program is expected to display no warning. | ||
|
|
||
| - 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. | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. on formatting: maybe emphase the keywords "Error/Warning/Info…"? |
||
|
|
||
| - Opened for discussion (as in general, a little bit less relevant for the desktop): use of structured logging library. | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I guess this is a note as a TODO in the end of the doc? |
||
|
|
||
| - 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. | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. use cases*? |
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think those 2 parts are actually separated. Especially the use of
gofumt(which is stricter thangofmt, but compatible: what pass gofumt will always pass gofmt. However, not all "gofmted" code will necessarily pass gofumt).