grapevine/book/contributing/style-guide.md

# Style guide

It is recommended that contributors follow this guide to minimize nitpicking on
their merge requests.

However, this guide is not set in stone. It is open to changes as new patterns
emerge, requirements change, compelling arguments are made, and so on. The goal
is to document the existing style so it can be applied consistently, not to
ensure the style never changes.

## Merge requests

When updating a merge request branch, use `git rebase`; do not create merge
commits to merge other branches into a merge request branch.

**Why?** This keeps the history simpler, and lacking merge commits makes it
easier to revert any individual commit.

## Commit messages

[Here's a good article][git-commit] on how to write good commit messages in
general.

Specifically for this project:

* Capitalizing the first letter is not required.
* It is recommended to avoid "conventional commits", as they take away from the
  very limited subject line length, and will not be processed in an automated
  fashion anyway.

**Why?** The linked article explains why this is good practice.

[git-commit]: https://cbea.ms/git-commit/

## Structuring commits

Try to structure each commit so that it falls into one of the following
categories:

* Refactoring, with no behavior change.
* Changing behavior, with no refactoring.
* Removing behavior, with no refactoring.
* Adding behavior, with no refactoring.
* Rewriting something completely. It is rare that these kinds of commits are
  warranted.

If you find yourself wanting to use the word "and" in the commit's subject line,
it should probably be broken into multiple commits.

During code review, it's common to get feedback requesting changes to your
commits. To apply this feedback, do not make and push a new commit containing
the requested change. Instead, include the requested change in the commit of
yours that gave rise to the suggestion. If you are unfamiliar with rewriting
history in git, [this website][git-rebase] is a great tutorial.

**Why?** Small, targeted, and well-explained commits make it easier for
reviewers to verify that a change has its intended effect. Or, for someone
running `git bisect` to find a more granular answer to why their test began
failing.

[git-rebase]: https://git-rebase.io/

## `mod`/`use` order

`mod` and `use` statements should appear in the following order, separated by
a blank line:

1. `use` statements referring to `std`, `alloc`, or `core`, if any.
2. `use` statements referring to other crates, if any.
3. `use` statements referring to `super` or `crate`, if any.
4. Macro definitions that need to be accessible from child modules, if any.
5. `mod` statements, if any.
6. `use` statements referring to modules declared by the above `mod` statements,
   if any.

`rust-analyzer` and `rustfmt` automate most of this except points 4 and 5.

**Why?** Consistency is good.

## Testing

When writing tests, be sure to keep the contents of [this article][cargo-test]
in mind. Especially, keeping Cargo unit tests in a dedicated tests file
(mentioned towards the end of the article).

**Why?** The linked article explains why this is good practice.

[cargo-test]: https://matklad.github.io/2021/02/27/delete-cargo-integration-tests.html

## Tracing

Modules that emit tracing events should import
`crate::observability::prelude::*`, which re-exports `tracing` as `t`, instead
of importing tracing macros directly and using them without a path.

**Why?** Avoiding name collisions and diff churn, since the set of tracing
macros used by a module tends to change frequently.

`tracing` events should:

1. Start with a capital letter (when applicable).

`tracing` events should not:

1. End with punctuation.
2. Interpolate values into the event's message.
   * Instead, add those values as structured fields.

**Why?** Consistency is good. Also, interpolating values into the event message
essentially defeats the point of structured logging.

When emitting tracing events containing errors, use the `<level>_err!` macros
from `observability::prelude` instead of `t::<level>!`.

**Why?** This will log the full source chain instead of just the `Display` impl
of the first error in the chain, making it easier to identify the cause of
errors in thelogs.

### Examples

#### 1

```rust,ignore
// This does not conform because it does not start with a capital letter.
t::info!("started pentametric fan");

// Do this instead:
t::info!("Started pentametric fan");
```

#### 2

```rust,ignore
// This does not conform because it ends with punctuation.
t::info!("Started pentametric fan.");

// Do this instead:
t::info!("Started pentametric fan");
```

#### 3

```rust,ignore
// This does not conform because it interpolates values into the event's
// message.
t::warn!("Noticed {} discombobulated waneshafts", count);

// Do this instead:
t::warn!(count, "Noticed discombobulated waneshafts");
```

#### 4

```rust,ignore
// This does not conform because it logs only the first error in the chain
t::error!(%error, "Failed to automatically synchronize cardinal grammeters");

// Do this instead:
error_err!(error, "Failed to automatically synchronize cardinal grammeters");
```

## Services

Services are abstraction units that live inside the `src/service` directory.

Calling service constructors must not cause side effects, with a few exceptions:

* Database reads.
* Local filesystem reads.

**Why?** This restriction makes it possible to implement subcommands that run
"offline" that reuse service code.