diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 31ea0a20..7f1a2347 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -3,20 +3,155 @@ Contributions are very welcome. When contributing code, please follow these simple guidelines. -* Make sure you run `rustfmt` on your code. Also ensure all trailing whitespace is trimmed. -* Run the tests with `cargo test --all`. -* Before adding any code dependencies, check with the maintainers if this is okay. -* Write properly formatted comments: they should be English sentences, eg: +* Follow the coding guidelines when proposing code changes ([see below](#code-style)). +* Write properly formatted git commits messages ([see below](#writing-git-commit-messages)). +* Read the DCO and if you are contributing a significant amount of code, make sure your commits are signed off, using `git commit -s`. - // Return the current UNIX time. +## Linting & formatting -* Read the DCO and make sure all commits are signed off, using `git commit -s`. -* Follow the guidelines when proposing code changes (see below). -* Write properly formatted git commits (see below). +Always check your code with the linter (`clippy`), by running: -Proposing changes ------------------ -When proposing changes via a pull-request or patch: + $ cargo clippy --all --tests + +And make sure your code is formatted with, using: + + $ cargo fmt + +Finally, ensure there is no trailing whitespace anywhere. + +## Running tests + +Make sure all tests are passing with: + + $ cargo test --all + +## Checking the docs + +If you make documentation changes, you may want to check whether there are any +warnings or errors: + + $ cargo doc --all --all-features + +## Code style + +The following code guidelines will help make code review smoother. + +### Use of `unwrap` and `expect` + +Use `unwrap` only in either of three circumstances: + +1. Based on manual static anaylsis, you've concluded that it's impossible for +the code to panic; so unwrapping is *safe*. An example would be: + + let list = vec![a, b, c]; + let first = list.first().unwrap(); + +2. The panic caused by `unwrap` would indicate a bug in the software, and it +would be impossible to continue in that case. + +3. The `unwrap` is part of test code, ie. `cfg!(test)` is `true`. + +In the first and second case, document `unwrap` call sites with a comment prefixed +with `SAFETY:` that explains why it's safe to unwrap, eg. + + // SAFETY: Node IDs are valid ref strings. + let r = RefString::try_from(node.to_string()).unwrap(); + +Use `expect` only if the function expects certain invariants that were not met, +either due to bad inputs, or a problem with the environment; and include the +expectation in the message. For example: + + logger::init(log::Level::Debug) + .expect("logger must only be initialized once"); + +### Module imports + +Modules are declared at the top of the file, before the imports. Public modules +are separated from private modules with a blank line: + + mod git; + mod storage; + + pub mod refs; + + use std::time; + use std::process; + + ... + +Imports are organized in groups, from least specific to more specific: + + use std::collections::HashMap; // First, `std` imports. + use std::process; + use std::time; + + use git_ref_format as format; // Then, external dependencies. + use once_cell::sync::Lazy; + + use crate::crypto::PublicKey; // Finally, local crate imports. + use crate::storage::refs::Refs; + use crate::storage::RemoteId; + +### Variable naming + +Use short 1-letter names when the variable scope is only a few lines, or the context is +obvious, eg. + + if let Some(e) = result.err() { + ... + } + +Use 1-word names for function parameters or variables that have larger scopes: + + pub fn commit(repo: &Repository, sig: &Signature) -> Result { + ... + } + +Use the most descriptive names for globals: + + pub const KEEP_ALIVE_DELTA: LocalDuration = LocalDuration::from_secs(30); + +### Function naming + +Stay concise. Use the function doc comment to describe +what the function does, not the name. Keep in mind functions are in the +context of the parent module and/or object and repeating that would be +redundant. + +### Logging + +When writing log statements, always include a `target` and include enough +context in the log message that it is useful on its own, eg. + + debug!(target: "service", "Routing table updated for {rid} with seed {nid}"); + +Check the file you are working on for what the `target` name should be; most +logs should be at the *debug* level. + +## Dependencies + +Before adding any code dependencies, check with the maintainers if this is okay. +In general, we try not to add external dependencies unless it's necessary. +Dependencies increase counter-party risk, build-time, attack surface, and +make code harder to audit. + +## Documentation + +Public types and functions should be documented. Modules *may* be documented, +if you see the need. + +Code comments should usually be full english sentences, and add missing context +for the reader: + + // Ensure that our inventory is recorded in our routing table, and we are tracking + // all of it. It can happen that inventory is not properly tracked if for eg. the + // user creates a new repository while the node is stopped. + for rid in self.storage.inventory()? { + ... + +## Proposing changes + +When proposing changes via a patch: * Isolate changes in separate commits to make the review process easier. * Don't make unrelated changes, unless it happens to be an obvious improvement to @@ -28,8 +163,8 @@ When proposing changes via a pull-request or patch: codebase, write a proposal in English first, and get consensus on that before proposing the code changes. -Writing Git commit messages ---------------------------- +## Writing Git commit messages + A properly formed git commit subject line should always be able to complete the following sentence: