Radish alpha
h
rad:z3gqcJUoA1n9HaHKufZs5FCSGazv5
Radicle Heartwood Protocol & Stack
Radicle
Git
Update CONTRIBUTING
Open fintohaps opened 3 months ago

A pass was made over the CONTRIBUTING.md file to update it to our most recent practices.

1 file changed +57 -26 d2ab7b1b 677b0dc0
modified CONTRIBUTING.md
@@ -14,6 +14,11 @@ If creating an issue, please make sure to include:
- the output of `rad debug`,
- the contents of the log files referred to by that output.

+
If possible, please reproduce the issue after setting the log level
+
for `radicle-node` to "debug". This is done by specifying the
+
commandline argument `--log-level debug`.
+
If you start `radicle-node` via `rad node start`, then change the
+
invocation to `rad node start -- --log-level debug`.

## Contributing Code

@@ -39,9 +44,9 @@ Patch formatting follows the same rules as commit formatting. See below.

Always check your code with the linter (`clippy`), by running:

-
    $ cargo clippy --workspace --tests
+
    $ cargo clippy --workspace --all-targets --all-features -- -Dwarnings

-
And make sure your code is formatted with, using:
+
And make sure your code is formatted using:

    $ cargo fmt

@@ -56,12 +61,22 @@ Make sure all tests are passing with:
Some tests require `jq`. If `jq` is not detected, these tests will succeed
without effectively testing anything.

+
#### `cargo nextest`
+

+
Another popular test runner is [`nextest`](https://nexte.st/), which can be used through `cargo nextest`.
+

+
The equivalent to the above `cargo test` command would be:
+

+
    $ cargo nextest run --workspace
+

+
Note that `cargo nextest` does not include doc tests, however, we do not write doc tests.
+

### Checking the docs

If you make documentation changes, you may want to check whether there are any
warnings or errors:

-
    $ cargo doc --workspace --all-features
+
    $ cargo doc --workspace --all-features --no-deps

### Code style

@@ -69,31 +84,42 @@ 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 analysis, you've concluded that it's impossible for
-
the code to panic; so unwrapping is *safe*. An example would be:
+
In general, `unwrap` or `expect` should be seldom used.
+
Instead, proper handling of these cases should made, whether through better expressing
+
the logic using types, through error handling, or a combination of these.

-
        let list = vec![a, b, c];
-
        let first = list.first().unwrap();
+
However, there are some instances where they are acceptable:

-
2. The panic caused by `unwrap` would indicate a bug in the software, and it
-
would be impossible to continue in that case.
+
1. In tests, where `#[allow(clippy::unwrap_used)]` is commonly used.
+
2. If, after analysing the code for yourself, you have concluded that
+
   a call to `unwrap` does not panic (not under any circumstance, for any given
+
   input). In this case, it is still preferred to use `expect` and provide
+
   a message which gives your argument.
+
   Note that if your analysis does not appear very stable under possible future
+
   refactorings and changes, it might still not be accepted.
+
3. The presence of `Option::None` or `Result::Err` is truly an unexpected scenario
+
   and you intend the program to panic.

-
3. The `unwrap` is part of test code, ie. `cfg!(test)` is `true`.
+
#### Use of the terms "safe" and "safety"

-
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.
+
As we are programming in Rust, the terms safe and unsafe are narrowly defined to
+
refer to the safety in the sense that is common to the Rust programming community,
+
i.e. "Safe Rust" and "Unsafe Rust", as mentioned in [The Rustonomicon].

-
    // SAFETY: Node IDs are valid ref strings.
-
    let r = RefString::try_from(node.to_string()).unwrap();
+
With this in mind, we only accept the use of "safe" about code if it is related
+
to implementing something within the realm of Unsafe Rust, as well as upholding
+
guarantees demanded by Safe Rust. The comment must describe why the use of
+
Unsafe Rust is in fact safe (i.e. which invariants are considered), and under
+
which conditions.

-
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:
+
We will not accept the use of "safe" to describe conditions under which code
+
will not panic, i.e. why it would be panic-free to use `unreachable!`, `unwrap`,
+
or `expect`. As we differentiate between "panic-free Rust" and Safe Rust.
+
We will, however, still recommend that those uses are documented – generally
+
using a `# Panics` header followed by an explanatory paragraph in a documenting
+
comment.

-
    logger::init(log::Level::Debug)
-
        .expect("logger must only be initialized once");
+
[The Rustonomicon]: https://doc.rust-lang.org/nomicon/meet-safe-and-unsafe.html

#### Module imports

@@ -119,10 +145,12 @@ Imports are organized in groups, from least specific to more specific:
    use git_ref_format as format;    // Then, external dependencies.
    use serde_json::Value;

-
    use crate::crypto::PublicKey;    // Finally, local crate imports.
+
    use crate::crypto::PublicKey;    // Then, local crate imports.
    use crate::storage::refs::Refs;
    use crate::storage::RemoteId;

+
    use super::Oid;                  // Finally, super imports.
+

#### Variable naming

Use short 1-letter names when the variable scope is only a few lines, or the context is
@@ -215,9 +243,11 @@ When proposing changes via a patch:

**Preparing commits**

-
1. Each commit in your patch must pass all the tests, lints and checks. This is
-
   so that they can be built into binaries and to make git bisecting possible.
-
2. Do not include any commits that are fixes or refactorings of previous patch
+
1. It is preferred that each commit in your patch passes all the tests, lints
+
   and checks. This is so that they can be built into binaries and to make git
+
   bisecting possible. There are times when it is necessary to have commits that
+
   do not pass checks, e.g. the commit would become too large.
+
2. Do not include any commits that are fixes or refactoring of previous patch
   commits. These should be squashed to the minimal diff required to make the
   change, unless it's helpful to make a large change over multiple commits,
   while still respecting (1). Do not include `fixup!` commits either.
@@ -225,7 +255,8 @@ When proposing changes via a patch:
   mainly concerns a certain area of the codebase. For example. These prefixes
   should usually be the name of the crate, minus any common prefix. Eg.
   `cli:`, and *not* `radicle-cli:`. For documentation, you can use `docs:`,
-
   and for CI-related files, you can use `ci:`.
+
   and for CI-related files, you can use `ci:`. Additionally, you can add
+
   further scope within the crate, e.g. `node/service`.

To help with the above, use `git commit --amend` and `git rebase -i`. You can
also interactively construct a commit from a working tree using `git add -p`.