337 lines
13 KiB
Markdown
337 lines
13 KiB
Markdown
# CONTRIBUTING
|
||
|
||
Contributions are very welcome.
|
||
|
||
## Contributing Issues
|
||
|
||
Contributing issues are an excellent way to ensure your problem will
|
||
eventually get looked at.
|
||
|
||
If creating an issue, please make sure to include:
|
||
- actual behaviour you're observing,
|
||
- the behaviour you were expecting instead,
|
||
- steps to reproduce your issue,
|
||
- 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
|
||
|
||
When contributing code, please follow these
|
||
simple guidelines.
|
||
|
||
* 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`.
|
||
* Contribute code via a Radicle Patch, by following the [guide][].
|
||
* If it's a feature addition or major change, check with the maintainers first
|
||
before submitting a patch. We wouldn't want you to waste your time!
|
||
* If you need help or would like to discuss your changes, come to our community chat on [Zulip][zulip].
|
||
|
||
[guide]: https://radicle.dev/guides/user#working-with-patches
|
||
[zulip]: https://radicle.zulipchat.com
|
||
|
||
### Submitting patches
|
||
|
||
Patch formatting follows the same rules as commit formatting. See below.
|
||
|
||
### Git hooks & Task runner
|
||
|
||
We use [`just >= v1.49.0`](https://just.systems/) as our task runner. You can see all available commands by running `just` or `just --list` in the repository root.
|
||
|
||
If you are not using Nix (which sets up hooks automatically), you should install the local git hooks:
|
||
|
||
$ just install-hooks
|
||
|
||
These hooks will run formatting, linting, and spelling checks on `pre-commit` and `pre-push`. For security, our hooks are copied rather than symlinked. If you check out a branch that modifies sensitive files (like `build.rs` or `justfile`), the hook will pause and ask for your confirmation before executing any code.
|
||
|
||
### Linting & formatting
|
||
|
||
Always check your code with the linter (`clippy`), by running:
|
||
|
||
$ just lint-rust
|
||
|
||
And make sure your code is formatted using:
|
||
|
||
$ just format-rust
|
||
|
||
You can also run the entire suite of pre-commit checks (which includes spelling and shell checks) with:
|
||
|
||
$ just pre-commit
|
||
|
||
Finally, ensure there is no trailing whitespace anywhere.
|
||
|
||
### Running tests
|
||
|
||
Make sure all tests are passing with:
|
||
|
||
$ cargo test --workspace
|
||
|
||
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.
|
||
|
||
To run the test suite, use the command:
|
||
|
||
$ just test-rust
|
||
|
||
### Checking the docs
|
||
|
||
If you make documentation changes, you may want to check whether there are any
|
||
warnings or errors:
|
||
|
||
$ just check-docs
|
||
|
||
### Code style
|
||
|
||
The following code guidelines will help make code review smoother.
|
||
|
||
#### Use of `unwrap` and `expect`
|
||
|
||
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.
|
||
|
||
However, there are some instances where they are acceptable:
|
||
|
||
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.
|
||
|
||
#### Use of the terms "safe" and "safety"
|
||
|
||
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].
|
||
|
||
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.
|
||
|
||
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.
|
||
|
||
[The Rustonomicon]: https://doc.rust-lang.org/nomicon/meet-safe-and-unsafe.html
|
||
|
||
#### 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 serde_json::Value;
|
||
|
||
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
|
||
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<Commit, Error> {
|
||
...
|
||
}
|
||
|
||
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()? {
|
||
...
|
||
|
||
### Referring to radicle.dev in Code
|
||
|
||
While <https://radicle.dev> is the main website of the project, and also the domain
|
||
associated with COBs implemented in this repo, we strive to write code that is as
|
||
independent as reasonably possible from this particular domain name. For example, it
|
||
should not be used for default configuration values, or if it is, there should be a
|
||
way to override.
|
||
|
||
This makes it easier to re-package Radicle for distribution under a different domain
|
||
or fork it altogether. It also tends to produce better, more flexible, code.
|
||
|
||
In tests, instead use names that are compliant with RFC 2606, e.g.
|
||
"radicle.example.com".
|
||
|
||
Note that as of 2025-08, there are still a few mentions of "radicle.dev" in the
|
||
codebase (mostly tests or user hints, fallback for configuration), and some of them
|
||
are not easy to remove. However, this is in no way a justification to add more
|
||
references.
|
||
|
||
In 2026-04, the project moved from "radicle.xyz" to "radicle.dev", and many IDs,
|
||
most notably COB type names and payload IDs (both in reverse domain name notation)
|
||
were not changed to stay backward compatible. So, while generally, "radicle.xyz"
|
||
should not be used anymore, think twice before changing such occurrences to
|
||
"radicle.dev". The same holds for "xyz.radicle", but in that case please think
|
||
thrice.
|
||
|
||
### 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
|
||
code you are touching anyway ("boyscout rule").
|
||
* Rebase on `master` when needed.
|
||
* Keep your changesets small, specific and uncontroversial, so that they can be
|
||
merged more quickly.
|
||
* If the change is substantial or requires re-architecting certain parts of the
|
||
codebase, write a proposal in English first, and get consensus on that before
|
||
proposing the code changes.
|
||
|
||
**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
|
||
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.
|
||
3. A commit *may* include a category prefix such as `cli:` or `node:` if it
|
||
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:`.
|
||
|
||
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`.
|
||
|
||
### Writing commit messages
|
||
|
||
A properly formed git commit subject line should always be able to complete the
|
||
following sentence:
|
||
|
||
If applied, this commit will _____
|
||
|
||
In addition, it should be capitalized and *must not* include a period.
|
||
|
||
For example, the following message is well formed:
|
||
|
||
Add support for .gif files
|
||
|
||
While these ones are **not**: `Adding support for .gif files`,
|
||
`Added support for .gif files`, `add support for .gif files`.
|
||
|
||
When it comes to formatting, here's a model git commit message[1]:
|
||
|
||
Capitalized, short (50 chars or less) summary
|
||
|
||
More detailed explanatory text, if necessary. Wrap it to about 72
|
||
characters or so. In some contexts, the first line is treated as the
|
||
subject of an email and the rest of the text as the body. The blank
|
||
line separating the summary from the body is critical (unless you omit
|
||
the body entirely); tools like rebase can get confused if you run the
|
||
two together.
|
||
|
||
Write your commit message in the imperative: "Fix bug" and not "Fixed bug"
|
||
or "Fixes bug." This convention matches up with commit messages generated
|
||
by commands like git merge and git revert.
|
||
|
||
Further paragraphs come after blank lines.
|
||
|
||
- Bullet points are okay, too.
|
||
|
||
- Typically a hyphen or asterisk is used for the bullet, followed by a
|
||
single space, with blank lines in between, but conventions vary here.
|
||
|
||
- Use a hanging indent.
|
||
|
||
---
|
||
|
||
### Using `direnv`
|
||
|
||
The team maintains an `.envrc.sample` file (see [direnv](https://direnv.net/)), that contributors may choose to copy or symlink to their local `.envrc` file.
|
||
This provides some basic tooling and setup that is common to the team.
|
||
For example, if `nix` is installed, the `flake.nix` and `rust-toolchain.toml` files are automatically watched for updates.
|
||
|
||
_NOTE: It is suggested you do not use `source_env .envrc.sample` in your `.envrc` as [`direnv`'s security checks](https://direnv.net/man/direnv-stdlib.1.html#codesourceenv-ltfileordirpathgtcode) are not triggered when changes are made to `.envrc.sample`._
|
||
|
||
[1]: http://tbaggery.com/2008/04/19/a-note-about-git-commit-messages.html
|