From c38d877b94c6ac5df2de4c9c939ae683d733e8b8 Mon Sep 17 00:00:00 2001 From: Owen Jacobson Date: Mon, 23 Jun 2025 22:45:18 -0400 Subject: Organize the developer docs into a "Pilcrow for Developers" book. The audience for this is developers looking to make changes to Pilcrow, either on the server, on the included client, or via its data model. Most of the material here is drawn from existing documents, but organized somewhat more coherently. I've left some space for client documentation, though no such documents exist yet. --- docs/debian-packaging.md | 8 ---- docs/design.md | 38 ----------------- docs/developer/SUMMARY.md | 20 +++++++++ docs/developer/book.toml | 10 +++++ docs/developer/design-guidance.md | 13 ++++++ docs/developer/introduction.md | 24 +++++++++++ docs/developer/server/code-organization.md | 25 +++++++++++ docs/developer/server/data-models.md | 68 ++++++++++++++++++++++++++++++ docs/developer/server/debian-packaging.md | 10 +++++ docs/developer/server/errors.md | 9 ++++ docs/developer/server/running.md | 35 +++++++++++++++ docs/developer/server/testing.md | 27 ++++++++++++ docs/developer/server/time.md | 30 +++++++++++++ docs/developer/tools/formatting.md | 29 +++++++++++++ docs/developer/tools/linting.md | 21 +++++++++ docs/formatting.md | 29 ------------- docs/linting.md | 21 --------- docs/mission.md | 3 -- docs/testing.md | 27 ------------ 19 files changed, 321 insertions(+), 126 deletions(-) delete mode 100644 docs/debian-packaging.md delete mode 100644 docs/design.md create mode 100644 docs/developer/SUMMARY.md create mode 100644 docs/developer/book.toml create mode 100644 docs/developer/design-guidance.md create mode 100644 docs/developer/introduction.md create mode 100644 docs/developer/server/code-organization.md create mode 100644 docs/developer/server/data-models.md create mode 100644 docs/developer/server/debian-packaging.md create mode 100644 docs/developer/server/errors.md create mode 100644 docs/developer/server/running.md create mode 100644 docs/developer/server/testing.md create mode 100644 docs/developer/server/time.md create mode 100644 docs/developer/tools/formatting.md create mode 100644 docs/developer/tools/linting.md delete mode 100644 docs/formatting.md delete mode 100644 docs/linting.md delete mode 100644 docs/mission.md delete mode 100644 docs/testing.md (limited to 'docs') diff --git a/docs/debian-packaging.md b/docs/debian-packaging.md deleted file mode 100644 index f307536..0000000 --- a/docs/debian-packaging.md +++ /dev/null @@ -1,8 +0,0 @@ -# Building Debian packages - -You will need `docker` installed, and set up to use the `containerd` storage backend. The builder image is built as a multi-architecture image. - -1. Run `tools/build-builder`. -2. Run `tools/build-debian`. - -Packages will be built in `target/debian`. diff --git a/docs/design.md b/docs/design.md deleted file mode 100644 index 47df384..0000000 --- a/docs/design.md +++ /dev/null @@ -1,38 +0,0 @@ -# Internal design - -`pilcrow`'s design is discovered and not planned. Do not take this as doctrine; continue to experiment on the structure as you find new needs. - -As of this writing, the basic flow of most requests hits several subsystems: - -- Axum handles parsing and delivering requests to endpoint handlers. -- Endpoint handlers interact with an "app" method (mostly defined in `**/app.rs`) to carry out logic. -- App methods interact with the database through "repo" methods. -- Repo methods access the database, and carry out minor tasks directly adjacent to data access (ID generation being the main example). - -This approach helps enable testing (see [testing.md] and the implementation of most of the tests), and helps maintain some conceptual separation between what this service does and how each piece individually works. - -## Time - -Handling time in a service is always tricky. - -`pilcrow` takes the approach that a request is considered to be serviced at one time, and that that time is determined when the request is received. The internals of `pilcrow` - the "app" and data access types described below, as well as most other supporting tools - are written with an eye towards accepting the "current time" as an argument, rather than calling out to a clock for themselves. - -The "current time" for a request is determined in `src/clock.rs`, which runs on every request, and is available to the handler via the `RequestedAt` extractor defined in that module. - -One of the perks of this approach is that tests, which call extractors, app interfaces, and data access interfaces directly, can pass any time they please, which enables scenarios like "if I sent a message last year, it should have expired by today." - -## Errors - -In general, errors are reported back up as Rust values with full Rust semantics, and are converted to HTTP errors at the last moment (generally via `IntoResponse` implementations on internal error types). Where possible, I've preferred using newtype to wrap an underlying error over defining multiple "similar" error taxonomies, but the error handling is somewhat inconsistent, so don't take this too seriously. - -The key property that _does_ matter is making sure the errors making it out of - -## Code organization - -This project uses a hybrid organization approach. - -High-level concerns are grouped into modules. These are `crate::channel`, `crate::events`, `crate::login`, and others. Those modules generally contain their own app types, their own repo types, their own extractors, and any other supporting code they need. They may also provide an interface to other modules in the program. - -However, some cross-cutting parts of the system have been pulled out into top-level modules of their own. These are `crate::repo`, `crate::app`, `crate::clock`, and others. Reasons for doing this include having multiple equally-valid modules the code could have been put in, keeping the use of namespaces legible, and others. - -Trust your gut, and reorganize to meet new needs. diff --git a/docs/developer/SUMMARY.md b/docs/developer/SUMMARY.md new file mode 100644 index 0000000..6b0ce6a --- /dev/null +++ b/docs/developer/SUMMARY.md @@ -0,0 +1,20 @@ +# Summary + +[Introduction](introduction.md) + +- [Design guidance](design-guidance.md) + +# The server + +- [Code organization](server/code-organization.md) +- [Data models](server/data-models.md) +- [Time](server/time.md) +- [Errors](server/errors.md) +- [Testing](server/testing.md) +- [Running Pilcrow locally](server/running.md) +- [Debian packaging](server/debian-packaging.md) + +# Development tools + +- [Formatting](tools/formatting.md) +- [Linting](tools/linting.md) diff --git a/docs/developer/book.toml b/docs/developer/book.toml new file mode 100644 index 0000000..ef1296c --- /dev/null +++ b/docs/developer/book.toml @@ -0,0 +1,10 @@ +[book] +title = "Pilcrow for Developers" +authors = ["Owen Jacobson"] +language = "en" +multilingual = false + +src = "." + +[build] +build-dir = "../../target/docs/developer" diff --git a/docs/developer/design-guidance.md b/docs/developer/design-guidance.md new file mode 100644 index 0000000..28269dd --- /dev/null +++ b/docs/developer/design-guidance.md @@ -0,0 +1,13 @@ +# Design guidance + +Pilcrow's design is discovered and not planned. Do not take existing design as doctrine; instead, change the design to accommodate new needs and past lessons. + +When making decisions about this design, consider the following: + +- The target audience for this service is individuals running servers for their local communities. The archtecture is deliberately designed to be tractable and easy to understand, and to be deployable on as wide a range of internet-connected systems as is feasible. + +- While we don't have a specific goal for scale, think tens to hundreds of people, not tens to hundreds of thousands of people. + +- The modern browser is a surprisingly capable and complex platform in its own right. It may not be the only client platform we expect people to use in the long term, but it's a solid foundation for a client today. + +- The one thing it has to do, pretty much no matter what, is deliver messages. When that's not possible, the only responsible way to deal with the situation is to inform users that the operation they're asking for will not be completed. diff --git a/docs/developer/introduction.md b/docs/developer/introduction.md new file mode 100644 index 0000000..0b6233d --- /dev/null +++ b/docs/developer/introduction.md @@ -0,0 +1,24 @@ +# Introduction + +This book is for developers looking to make changes to Pilcrow, either on the server, on the included client, or via its data model. + +Pilcrow is a web-based chat system that enables users to send and receive short messages with (hopefully) low latency to other users within the same community. It is composed of two main parts: the Pilcrow server, and Pilcrow clients. The following chapters lay out how each of these components is organized and implemented. + +A Pilcrow deployment consists of one server, servicing any number of connected clients. + +## The Pilcrow server + +The Pilcrow server is a Unix application designed to run as a service. It exposes services to Pilcrow users via an HTTP API, both for synchronous operations, and for asynchronous notifications. The primary services are: + +- An _event stream_, notifying client applications of operations carried out by other clients or by the server, and +- A collection of _API endpoints_, allowing client applications to submit operations to the server. + +The Pilcrow server also includes a Pilcrow client application, served over HTTP. + +For ease of deployment and administration, the Pilcrow server stores persistent data in a single database file, on the server's local filesystem. + +## Clients + +A Pilcrow client consists of one or more applications that connect to the Pilcrow server on behalf of one or more users. A client must subscribe to and consume events, and may submit operations back to the server over its HTTP API. + +The included client is a [progressive web application](https://developer.mozilla.org/en-US/docs/Web/Progressive_web_apps), which runs in most browsers that can run Javascript and provide complex layouts. When run, it connects to the server from which it was downloaded. It can also be installed on any OS or using any browser that supports local installation of progressive web applications. diff --git a/docs/developer/server/code-organization.md b/docs/developer/server/code-organization.md new file mode 100644 index 0000000..d17b604 --- /dev/null +++ b/docs/developer/server/code-organization.md @@ -0,0 +1,25 @@ +# Code organization + +This document explains the rationale for the server's module structure and overall code layout, rather than the layout. If you're looking for a map, look in `src/lib.rs`. + +Trust your gut, and reorganize to meet new needs. We've already revised this scheme several times as the code base has grown, and there's no reason we can't do so again. + +## Topic modules + +High-level concerns are grouped into topical modules. These include `crate::channel`, `crate::events`, `crate::login`, and others. Those modules generally contain their own app types, their own repo types, their own extractors, and any other supporting code they need. They may also provide an interface to other modules in the program. + +Most topic modules contain one or more of: + +- An `app` module exposing the primary operations on the topic domain, +- An `id` module defining any domain-specific identifier types, +- A `handlers` module exposing Axum handler functions, +- A `repo` module exposing database access types, +- A `history` module exposing history types, +- A `snapshot` module exposing state types, or +- An `events` module exposing domain-specific event types. + +Not every topic modules hits every common convention, and some topic modules vary from the conventions to meet their specific design needs. + +## Cross-cutting modules + +However, some cross-cutting parts of the system have been pulled out into top-level modules of their own. These include `crate::routes`, `crate::app`, `crate::clock`, and others. Reasons for doing this include having multiple equally-valid modules the code could have been put in, keeping the use of namespaces legible, and others. diff --git a/docs/developer/server/data-models.md b/docs/developer/server/data-models.md new file mode 100644 index 0000000..f7d42dd --- /dev/null +++ b/docs/developer/server/data-models.md @@ -0,0 +1,68 @@ +# Data models + +## Synchronized data + +Pilcrow is primarily a synchronization service for conversations, messages, and other social data. That synchronization is accomplished by expressing all changes to social data in terms of events, which are generated in response to user actions, recorded in sequence by Pilcrow, and published back to users. + +The Pilcrow server breaks events up into _histories_, internally. A history is a sequence of events that determines the state of a single entity, which can be considered in isolation from other events occurring within the system to make the majority of decisions. For example, a message's history contains everything you need to know to reconstruct a message at any point in time, and to make decisions about operations like deleting or editing the message, since those decisions do not depend on the overall state of the service. + +No model is perfect, and some operations will inevitably need to consider multiple histories. In general, this is acceptable if it's exceptional; however, if two histories end up consulted together on a regular basis, it may be worth reviewing whether they should be combined into a single history. Equally, histories can be split up, if they have been combined in ways that turn out to be less useful. + +### History storage + +The underlying storage generally reflects the specific structure of the kind of history it deals with, rather than being general. Operatiors and developers should be able to reason about a history directly from the single row, or the small set of rows, needed to reconstruct it. + +For example, a message history is represented by: + +- A row in `message`, which contains the information needed to reconstruct the `message` `send` event, and +- Optionally, a row in `message_deleted`, which can be joined to `message` to provide the information eeded to reconstruct a `message` `deleted` event if the message has actually been deleted. + +The resulting stored data reads very much like a classic, state-based relational model. `select * from message left join message_deleted using (id) where id = 'Mw9j2ttdx1f68k71'` gives message `Mw9j2ttdx1f68k71` in full detail: + +``` + id = Mw9j2ttdx1f68k71 + conversation = Cshp86chf87fypf6 + sender = L3twnpw918cftfkh + sent_sequence = 4839 + sent_at = 2025-06-02T05:49:36.418527558+00:00 + body = Oof those headings are too big + last_sequence = 4839 +deleted_sequence = + deleted_at = +``` + +These rows can be further joined to other rows in the database. Pilcrow's schema takes advantage of this for referential integrity checks, where possible, as well. Similar patterns arise in the implementation of other history types. + +### History operations + +A History type generally exposes a few key operations: + +- It can be loaded from an appropriate repository. This is usually provided by the repository interface, rather than by the history type itself. + +- It can be converted into a sequence of events (generally via an `events()` method), which can then be combined with other histories' events to provide a service-wide view of historical events. + +- It can produce snapshots of the underlying entity (generally via methods like `as_created()`, `as_of(instant)`, `as_snapshot()`, and similar) at various points in time. + +However, there is not, at this time, a common trait or type used for all histories, as no code in Pilcrow tries to abstract over them. We've found that generalizations tend to happen at the level of event sequences or streams, rather than at the level of the histories used to produce those sequences. + +### Soft and hard deletes + +One consequence of using events to synchronize data is that the service _must_ retain enough information to tell clients that a given thing has been deleted, at least temporarily, so that clients can remain in synch. This shows up in histories, and is even illustrated above: a deleted entity must have a non-deleted history, or the service cannot tell clients about deletion after the fact. + +To limit data growth, the server hard-deletes ("purges") histories for deleted entities after a while. After this happens, clients can no longer synchronize old state for that entity. Clients that get into this state must start over from the beginning of the event stream and rebuild their state. + +To manage social responsibility, the server aggressively discards data in live histories for deleted entities, as well. For example, a message's body is blanked when the message is deleted. Clients who synchronize with the server after that point will see a `message` `sent` event, but rather than containing the original body, it will contain a placeholder. In general, users who delete entities expect the server to stop disclosing the contents of those entities, even retroactively, and this compromise accomplishes that. + +### Paths not taken: generic event storage + +One reasonable response to the observation that Pilcrow is a synchronization service built on events is to propose storing events as the basic data primitive. When the server does need to make decisions based on moment-in-time state, that state can be derived from the stored events in the same way a Pilcrow client is expected to derive its own state. That has a pleasant symmetry, and would work. + +However, prior experience suggests that a data model consisting of an undifferentiated list of events is difficult for developers to work with. For clients, we've chosen to adopt this burden, in return for keeping Pilcrow's API orthogonal and reducing the benefit of being the "first-party" client. For the server, however, it is useful for developers to be able to determine at a glance what the state of a specific entity is, without reference to the whole recorded history. + +Prior experience also suggests that the same pressure acts on operators. While it's not built for this purpose, the underlying database _is_ necessarily a user interface, facing operators, who will occasionally opt to intervene in it to try to address operational needs or fix problems. They do so with incomplete understanding of the system, so the more the data model can guide them in the direction of correct changes, the more useful it is and the more successful they will be. + +And, finally, storing an event stream makes checking the data for consistency, both in the moment and after the fact, _very_ difficult. One cannot, for example, express the constraint that a message event is only valid if the conversation it addresses exists, whereas this constraint is utterly mundane in noun-based "current state only" data models. + +## Non-synchronized data + +Some data (as of this writing, invites and authentication tokens) are not synchronized to clients. Instead, Pilcrow uses a more classical data modelling approach of storing only their current state, or nothing, in its database, and operates only on the current state regardless of the passage of time. diff --git a/docs/developer/server/debian-packaging.md b/docs/developer/server/debian-packaging.md new file mode 100644 index 0000000..eedced7 --- /dev/null +++ b/docs/developer/server/debian-packaging.md @@ -0,0 +1,10 @@ +# Debian packaging + +## Building Debian packages + +You will need `docker` installed, and set up to use the `containerd` storage backend. The builder image is built as a multi-architecture image. + +1. Run `tools/build-builder`. +2. Run `tools/build-debian`. + +Packages will be built in `target/debian`. diff --git a/docs/developer/server/errors.md b/docs/developer/server/errors.md new file mode 100644 index 0000000..e5cdafb --- /dev/null +++ b/docs/developer/server/errors.md @@ -0,0 +1,9 @@ +# Errors + +In general, errors are reported back up as Rust values with full Rust semantics, and are converted to HTTP errors at the last moment (generally via `IntoResponse` implementations on internal error types). + +## Errors and HTTP responses + +Handlers must make sure that the errors returned to clients over HTTP do not leak information about the server unintentionally. For example, errors due to misconfigured permissions should not cause Pilcrow to start informing clients of the specific filesystem path Pilcrow is configured to look for its database at: that information is not useful to most users, and may be used to attack the service or mislead the operator. To that end, handlers, middleware, response types, and other HTTP-facing components are very explicit about what errors correspond to what responses. + +The fallback option, universally, is `crate::error::Internal`, which captures the underlying error and forwards it to standard error, but does not pass it along to HTTP clients. Every intended error is also part of Pilcrow's API documentation. diff --git a/docs/developer/server/running.md b/docs/developer/server/running.md new file mode 100644 index 0000000..27fdf4a --- /dev/null +++ b/docs/developer/server/running.md @@ -0,0 +1,35 @@ +# Running Pilcrow locally + +## Running a freestanding server + +Pilcrow can be run "as if" it were a real deployment using Cargo: + +```bash +cargo run +``` + +or + +```bash +cargo run --release +``` + +Cargo will build the server, and the UI if necessary, and package them together into a single binary. Options can be passed to the server using `cargo run -- […options…]`. + +## Running an editable client build + +It can be useful to run the client as a separate application during development. SvelteKit and Vite provide services such as live reloading that significantly streamline client development. To do this, start a local server (or see below), then separately start the client with Vite: + +```bash +npx vite dev +``` + +Vite will proxy API requests through to the running Pilcrow server, but provide its own view of the UI. + +Using the `tools/run` script will run both the Pilcrow server and a development client build. + +The client can also be run against other servers by setting the `API_SERVER` environment variable to the _base_ URL of the server: + +```bash +API_SERVER=https://hi.grimoire.ca/ npx vite run +``` diff --git a/docs/developer/server/testing.md b/docs/developer/server/testing.md new file mode 100644 index 0000000..8e87568 --- /dev/null +++ b/docs/developer/server/testing.md @@ -0,0 +1,27 @@ +# Testing + +Run `cargo test`. + +Most of the tests for this project use an integration-based testing approach: + +- Spin up a scratch copy of the application, using an in-memory database; +- Perform operations against that database using the `app` abstraction to set up the scenario; +- Interact with route endpoint functions directly to perform the test; +- Inspect both the return values from the endpoint function to validate the results; and +- Perform additional operations against the scratch database using the `app` absraction to validate behaviour. + +Tests can vary around this theme. + +Rather than hard-coding test data, tests use `faker_rand` and similar tools to synthesize data on a run-by-run basis. While this makes tests marginally less predictable, it avoids some of the issues of bad programmer-determined static testing data in favour of bad programmer-determined random testing data. Improvements in this area would be welcome - `proptest` would be ideal. + +The code for carrying out testing interactions lives in `src/test/fixtures`. By convention, only `crate::test::fixtures` is imported, with qualified names from there to communicate the specific kinds of fixtures that a test is using. + +## Style + +Prefer writing "flat" fixtures that do one thing, over compound fixtures that do several, so that the test itself conveys as much information as possible about the scenario under test. For example, even though nearly all tests involve an app with at least one user provisioned, there are separate fixtures for creating a scratch app and provisioning a user, and no fixture for "an app and its main user." + +Prefer role-specific names for test values: use, for example, `sender` for a login related to sending messages, rather than `login`. Fixture data is cheap, so make as many entities as make sense for the test. They'll vanish at the end of the test anyways. + +Prefer testing a single endpoint at a time. Other interactions, which may be needed to set up the scenario or verify the results, should be done against the `app` abstraction directly. It's okay if this leads to redundant tests (see for example `src/channel/routes/test/on_send.rs` and `src/events/routes/test.rs`, which overlap heavily). + +Panicking in tests is fine. Panic messages should describe why the preconditions were expected, and can be terse. diff --git a/docs/developer/server/time.md b/docs/developer/server/time.md new file mode 100644 index 0000000..2daf4e4 --- /dev/null +++ b/docs/developer/server/time.md @@ -0,0 +1,30 @@ +# Time + +Handling time in a service is always tricky. Pilcrow splits the problem into two parts: determining a single, consistent timeline for events, and determining what wall time a given operation happened at. + +## Sequence numbers + +The Pilcrow service tracks [events](data-models.md) in sequence. To accomplish this, the server assigns unique _sequence numbers_ to each event as it happens. Events are always considered in sequential order. + +Sequence number assignment is mediated through the `event_sequence` table in Pilcrow's database. Sequence number assignment must happen strictly serially, so this is a potential capacity and concurrency risk to anything that generates new events. In testing, this did not create any practical problems, but it likely contributes to Pilcrow's ultimate throughput limits. + +Sequence numbers are little more than 64-bit integers under the hood. The server is permitted to work with that representation directly, but we make no such promise in the API and expect clients to treat them as opaque values wherever they appear. Sequence numbers were at one point a more complex data structure, for example, and no client changes were made to accommodate the switch to serial numbers. If we move away from serial numbers to something else, we intend to follow the same rule. + +The server can record slightly more than 9.2 quintillion events before its sequence numbers will be exhausted. To put the magnitude of this range into perspective, a service processing 1k events per second - a rate far higher than Pilcrow is designed for - would run out of sequence numbers after a bit less than 300 million years. + +## Wall time + +Pilcrow takes the approach that a request is considered to be serviced at one specific point time, and that that time is determined when the request is received, regardless of how long it takes for that request to settle. + +The "current time" for a request is determined via an Axum middleware defined in `src/clock.rs`. The resulting time is available to handlers via the `RequestedAt` extractor. This time is always given in UTC, regardless of the time zone the server or client is running in. We expect administrators to keep accurate, or at least consistent, time; NTP is a good solution for this, as it's deployed by default on most popular server operating systems. + +Everything on the server that operates on time accepts the "current" time as a parameter. We consistently avoid making any other calls out to clocks, whether in Rust or through SQL constructs like +`NOW()`. + +One of the perks of this approach is that tests, which call extractors, app interfaces, and data access interfaces directly, can pass any time they please, rather than being dependent on an external clock. This enables testing scenarios like "if I sent a message last year, it should have expired by today." It also minimizes the risk of chronological anomalies due to clock changes mid-request, whether due to the normal passage of time or otherwise. + +## Warning + +**Pilcrow does not require that events happen in chronological order.** + +Requests that arrive close together may be assigned timestamps in one order, and assigned event sequence numbers in some other order, depending on the vagaries of how the concurrent requests are scheduled. Code that needs to reason about ordering _must not_ use wall time to determine order. diff --git a/docs/developer/tools/formatting.md b/docs/developer/tools/formatting.md new file mode 100644 index 0000000..90044eb --- /dev/null +++ b/docs/developer/tools/formatting.md @@ -0,0 +1,29 @@ +# Formatting + +We use automated tools, rather than human effort, to maintain a consistent code formatting convention, where possible. This is handled by two tools: + +- Javascript, Markdown, and JSON files are formatted using [prettier]. +- Rust is formatted using [rustfmt]. + +[prettier]: https://prettier.io +[rustfmt]: https://doc.rust-lang.org/cargo/commands/cargo-fmt.html + +## Tools + +- To check that code formatting rules are being followed, run + `tools/check-format`. This should be run any time you're making changes, and is part of the optional + `git-hooks/pre-commit` hook script. + +- To reformat the whole project, run `tools/reformat`. + +You can also run the individual formatting tools directly. The tool scripts listed above contain the specific commands needed. + +## Formatting and code review + +Checking in changes produced solely using +`tools/reformat` or its constituent commands doesn't require review if it's the only thing you're doing. We assume that these tools make sound changes, and that the code style configured for the project is acceptable by default. + +However, formatting changes carry an outsized risk of merge conflicts, so they almost always require +_coordination_. This risk is more severe for larger formatting changes. If you make a formatting fix, it's incumbent on you to follow up on it and to assist other developers with conflicts. + +In general, we try to avoid these by fixing formatting issues before they pile up, but if you plan to commit a formatting fix, please check for outstanding work that may conflict with it. You may even want to delay or rescope formatting fixes to accommodate others' in-progress changes. diff --git a/docs/developer/tools/linting.md b/docs/developer/tools/linting.md new file mode 100644 index 0000000..47420bb --- /dev/null +++ b/docs/developer/tools/linting.md @@ -0,0 +1,21 @@ +# Linting + +We use automated tools, rather than human effort, to spot possible bugs or dubious structural choices, where possible. This is handled by three tools: + +- Javascript is linted using [eslint]. +- Rust is linted using [cargo check] and [clippy]. + +[eslint]: https://eslint.org/ +[cargo check]: https://doc.rust-lang.org/cargo/commands/cargo-check.html +[clippy]: https://doc.rust-lang.org/cargo/commands/cargo-clippy.html + +## Tools + +- To check for detectable lints, run + `tools/check-lint`. This should be run whenever making changes, and is part of the optional + `git-hooks/pre-commit` hook script. + +- To fix lints that have automatic fixes, run + `tools/delint`. + +You can also run the individual lint tools directly. The tool scripts listed above contain the specific commands needed. diff --git a/docs/formatting.md b/docs/formatting.md deleted file mode 100644 index 90044eb..0000000 --- a/docs/formatting.md +++ /dev/null @@ -1,29 +0,0 @@ -# Formatting - -We use automated tools, rather than human effort, to maintain a consistent code formatting convention, where possible. This is handled by two tools: - -- Javascript, Markdown, and JSON files are formatted using [prettier]. -- Rust is formatted using [rustfmt]. - -[prettier]: https://prettier.io -[rustfmt]: https://doc.rust-lang.org/cargo/commands/cargo-fmt.html - -## Tools - -- To check that code formatting rules are being followed, run - `tools/check-format`. This should be run any time you're making changes, and is part of the optional - `git-hooks/pre-commit` hook script. - -- To reformat the whole project, run `tools/reformat`. - -You can also run the individual formatting tools directly. The tool scripts listed above contain the specific commands needed. - -## Formatting and code review - -Checking in changes produced solely using -`tools/reformat` or its constituent commands doesn't require review if it's the only thing you're doing. We assume that these tools make sound changes, and that the code style configured for the project is acceptable by default. - -However, formatting changes carry an outsized risk of merge conflicts, so they almost always require -_coordination_. This risk is more severe for larger formatting changes. If you make a formatting fix, it's incumbent on you to follow up on it and to assist other developers with conflicts. - -In general, we try to avoid these by fixing formatting issues before they pile up, but if you plan to commit a formatting fix, please check for outstanding work that may conflict with it. You may even want to delay or rescope formatting fixes to accommodate others' in-progress changes. diff --git a/docs/linting.md b/docs/linting.md deleted file mode 100644 index 47420bb..0000000 --- a/docs/linting.md +++ /dev/null @@ -1,21 +0,0 @@ -# Linting - -We use automated tools, rather than human effort, to spot possible bugs or dubious structural choices, where possible. This is handled by three tools: - -- Javascript is linted using [eslint]. -- Rust is linted using [cargo check] and [clippy]. - -[eslint]: https://eslint.org/ -[cargo check]: https://doc.rust-lang.org/cargo/commands/cargo-check.html -[clippy]: https://doc.rust-lang.org/cargo/commands/cargo-clippy.html - -## Tools - -- To check for detectable lints, run - `tools/check-lint`. This should be run whenever making changes, and is part of the optional - `git-hooks/pre-commit` hook script. - -- To fix lints that have automatic fixes, run - `tools/delint`. - -You can also run the individual lint tools directly. The tool scripts listed above contain the specific commands needed. diff --git a/docs/mission.md b/docs/mission.md deleted file mode 100644 index ab86fd9..0000000 --- a/docs/mission.md +++ /dev/null @@ -1,3 +0,0 @@ -# Mission - -The one thing it has to do, pretty much no matter what, is deliver messages. If it can't do that, it should be down, so that nobody thinks it's delivering messages. diff --git a/docs/testing.md b/docs/testing.md deleted file mode 100644 index 8e87568..0000000 --- a/docs/testing.md +++ /dev/null @@ -1,27 +0,0 @@ -# Testing - -Run `cargo test`. - -Most of the tests for this project use an integration-based testing approach: - -- Spin up a scratch copy of the application, using an in-memory database; -- Perform operations against that database using the `app` abstraction to set up the scenario; -- Interact with route endpoint functions directly to perform the test; -- Inspect both the return values from the endpoint function to validate the results; and -- Perform additional operations against the scratch database using the `app` absraction to validate behaviour. - -Tests can vary around this theme. - -Rather than hard-coding test data, tests use `faker_rand` and similar tools to synthesize data on a run-by-run basis. While this makes tests marginally less predictable, it avoids some of the issues of bad programmer-determined static testing data in favour of bad programmer-determined random testing data. Improvements in this area would be welcome - `proptest` would be ideal. - -The code for carrying out testing interactions lives in `src/test/fixtures`. By convention, only `crate::test::fixtures` is imported, with qualified names from there to communicate the specific kinds of fixtures that a test is using. - -## Style - -Prefer writing "flat" fixtures that do one thing, over compound fixtures that do several, so that the test itself conveys as much information as possible about the scenario under test. For example, even though nearly all tests involve an app with at least one user provisioned, there are separate fixtures for creating a scratch app and provisioning a user, and no fixture for "an app and its main user." - -Prefer role-specific names for test values: use, for example, `sender` for a login related to sending messages, rather than `login`. Fixture data is cheap, so make as many entities as make sense for the test. They'll vanish at the end of the test anyways. - -Prefer testing a single endpoint at a time. Other interactions, which may be needed to set up the scenario or verify the results, should be done against the `app` abstraction directly. It's okay if this leads to redundant tests (see for example `src/channel/routes/test/on_send.rs` and `src/events/routes/test.rs`, which overlap heavily). - -Panicking in tests is fine. Panic messages should describe why the preconditions were expected, and can be terse. -- cgit v1.2.3