Keyboard shortcuts

Press ← or → to navigate between chapters

Press S or / to search in the book

Press ? to show this help

Press Esc to hide this help

Developer Reference

The main requirement is to have the rust toolchain installed including cargo and rustfmt. This can be installed and configured via rustup and following the guide for your OS.

The main project runs on recent (1.75+) stable versions of rust but the code-style checks require a nightly version to also be available (see below).

Building

Once the build toolchain is available, all components can be built using cargo.

$ git clone git@gitlab.diamond.ac.uk:daq/amygdala.git
Cloning into 'amygdala'...
remote: Enumerating objects: 2071, done.
remote: Counting objects: 100% (2071/2071), done.
remote: Compressing objects: 100% (949/949), done.
remote: Total 2071 (delta 1190), reused 1800 (delta 1049), pack-reused 0
Receiving objects: 100% (2071/2071), 585.96 KiB | 25.48 MiB/s, done.
Resolving deltas: 100% (1190/1190), done.
$ cd amygdala
$ cargo build --workspace
// ... Compilation output removed
   Compiling amygdala v0.2.0-dev (/path/to/amygdala/amygdala)
   Compiling gda v0.2.0-dev (/path/to/amygdala/gda)
   Compiling amygdala-api v0.1.2 (/path/to/amygdala/amygdala-api)
    Finished dev [unoptimized + debuginfo] target(s) in 1m 38s
$

This will build both gda and amygdala into the ./target/debug/ directory.

Code style

The project should conform to the rustfmt code-style including the settings provided in rustfmt.toml in the project root (these will automatically be used when running cargo fmt).

As these options are still unstable in rustfmt it requires the nightly version of rust so should be run as cargo +nightly fmt.

rustfmt features used requiring nightly features

  • imports_granularity This restricts import nesting to one level, eg

    use foo::{a, b, c};
use foo::d::{e, f};

    instead of

    use foo::{a, b, c, d::{e, f}};

    which can quickly become unwieldy and hard to follow.

  • group_imports This reorders imports so that they are grouped by std library, third-party dependencies and crate level internal imports.

    This makes it easier to determine where imports are coming from and can differentiate between modules and external libraries.

All other configuration options are left as their default.

Lints and Warnings

Wherever possible, code should address any warnings or suggestions from the compiler or the default settings of cargo clippy.

Use of unwrap

The only additional, non-default restriction enforced is the inclusion of the unwrap_used = "deny" lint setting.

This lint prevents unwrap() being used on any Option or Result in the codebase. Panics cause a very negative experience for users and, with the default error message, it is not immediately obvious which pre-condition was not met.

If it is not possible to handle the error state cleanly or there are reasons why the unwrap can never fail, expect() should be used instead with the justification for why it is safe to do so as the message. While it doesn't prevent the hard-crash of the application, it does at least make it obvious which assumption was incorrect.

Project wide lint settings can be set in the root level Cargo.toml file. Eg to set the unwrap_used option 
[workspace.lints.clippy]
unwrap_used = "deny"
All projects include these lints by the inclusion of 
[lints]
workspace = true
in their own `Cargo.toml` file.

Git workflow

All additions to any of the projects should be made through PRs in gitlab. Commits should be reduced to logical steps with commits being rebased and reworded to provide useful context when viewed via git log without having to reference gitlab issues or PRs. The PR message can be more conversational and don't always make good commit messages.

If commits are squashed by gitlab when a PR is merged, care should be taken to ensure the generated message is still descriptive and doesn't just contain the PR title.

PRs in gitlab are required to be fast-forward only and merging a PR will not preserve the feature branch. If a PR consists of multiple commits where intermediate stages do not leave the repo in a working state. The PR should be for the merge commit to keep the history in a form that keeps the main branch in a buildable state. If a PR includes multiple commits including things like 'fix typos' and 'sort clippy lints', they should be squashed, either when submitting the PR or manually by the author.

Unfortunately, gitlab does not offer settings to enforce these preferences so care should be taken to keep the history coherent.

Commit messages

All commit messages should be useful and provide context without requiring access to gitlab (issues can be be linked in the text if required but should not be the entire content). A good style guide for commit messages is here. The main points:

  • Short single line summary (< 50 characters if possible)
  • Capitalise summary and use imperitive tense
  • Split summary and body with a blank line
  • Wrap all lines at 72 characters
  • Explain what and why rather than how

Changelog

All changes that affect either users or future developers should be included in the changelog of the relevant project. The entries should be added to the unreleased section to one of the subheadings listed in the Keep A Changelog guide:

  • Added for new features.
  • Changed for changes in existing functionality.
  • Deprecated for soon-to-be removed features.
  • Removed for now removed features.
  • Fixed for any bug fixes.
  • Security in case of vulnerabilities.

Changelog entries should be targeted more at users than commit messages are and copying the commit message verbatim into the changelog is rarely useful.

Review Checklist

Feel free to review any PR, not only those to which you have been added as a reviewer. While some minor checks do not require thorough checks (typos, reformatting etc), a good starting point for reviews would be

  • Is the change needed? This should probably have been covered in an issue before hand but it's always good to check.
  • Is this the best approach? Look at the higher level design first. Is the change being made to the right level of abstraction in the right area of the codebase?
  • Does it work? Ideally checkout the change (glab CLI can be useful) and test it locally rather than relying on the gitlab diff view.
  • Code level review Are there better ways of doing it? Could it be more optimised (only if not at the cost of clarity and functionality)? Are there more idiomatic approaches?
  • Are the docs still up to date?
  • Is there an entry in the changelog? If the change is going to be noticeable by someone using the application, it should be mentioned. If it is a very minor change and there is already a comment for a similar change, this may not be required, eg changes to help messages could all be covered by a single, "Improved CLI help text", entry.
  • Is it tested? There are very few tests in the project so far but it would be good if new changes could be tested, especially if they're fixing bugs that could be unintentionally reintroduced in future.