From 2a42df278a176c9cd0705d67447b69ff5feb11de Mon Sep 17 00:00:00 2001 From: Andrew Lamb Date: Mon, 22 Jun 2020 16:41:36 -0400 Subject: [PATCH] docs: Initial style guide with idomatic error handling (#174) * docs: Initial style guide with idomatic error handling * fix: Apply suggestions from code review Co-authored-by: Paul Dix * fix: Apply suggestions from code review Co-authored-by: Jake Goulding Co-authored-by: Carol (Nichols || Goulding) <193874+carols10cents@users.noreply.github.com> * fix: clean up example To not to use different field name Co-authored-by: Paul Dix Co-authored-by: Jake Goulding Co-authored-by: Carol (Nichols || Goulding) <193874+carols10cents@users.noreply.github.com> --- README.md | 3 + docs/style_guide.md | 189 ++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 192 insertions(+) create mode 100644 docs/style_guide.md diff --git a/README.md b/README.md index 598c4fa433..b54158eb5f 100644 --- a/README.md +++ b/README.md @@ -6,3 +6,6 @@ To compile and run this, you'll need: - The [`flatc` flatbuffer compiler](https://google.github.io/flatbuffers/flatbuffers_guide_building.html) >= 1.12.0 - `.env` file in the directory to specify configuration options. You can see an example in the `env` file. + +## Contributing: +- Delorean is written mostly in idiomatic Rust -- please see the [Style Guide](docs/style_guide.md) for more details. diff --git a/docs/style_guide.md b/docs/style_guide.md new file mode 100644 index 0000000000..d58a348a96 --- /dev/null +++ b/docs/style_guide.md @@ -0,0 +1,189 @@ +# Rationale and Goals +As every Rust programmer knows, the language has many powerful features, and there are often several patterns which can express the same idea. Also, as every professional programmer comes to discover, code is almost always read far more than it is written. + +Thus, we choose to use a consistent set of idioms throughout our code so that it is easier to read and understand for both existing and new contributors. + + + +## Errors + +### All errors should follow the [SNAFU crate philosophy](https://docs.rs/snafu/0.6.8/snafu/guide/philosophy/index.html) and use SNAFU functionality + +*Good*: + +* Derives `Snafu` and `Debug` functionality +* Has a useful, end-user-friendly display message + +```rust +#[derive(Snafu, Debug)] +pub enum Error { + #[snafu(display(r#"Conversion needs at least one line of data"#))] + NeedsAtLeastOneLine, + // ... +} +``` + +*Bad*: +```rust +pub enum Error { + NeedsAtLeastOneLine, + // ... +``` + + + +### Use the `ensure!` macro to check a condition and return an error + +*Good*: + +* Reads more like an `assert!` +* Is more concise +```rust +ensure!(!self.schema_sample.is_empty(), NeedsAtLeastOneLine); +``` + +*Bad* +```rust +if self.schema_sample.is_empty() { + return Err(Error::NeedsAtLeastOneLine {}); +} +``` + + +### Errors should be defined in the module they are instantiated + + + +*Good*: + +* Groups related error conditions together most closely with the code that produces them +* Reduces the need to `match` on unrelated errors that would never happen + +```rust +#[derive(Debug, Snafu)] +pub enum Error { + #[snafu(display("Not implemented: {}", operation_name))] + NotImplemented { operation_name: String } +} +// ... +ensure!(foo.is_implemented(), NotImplemented { + operation_name: "foo", +} +``` + +*Bad* +```rust +use crate::errors::NotImplemented; +// ... +ensure!(foo.is_implemented(), NotImplemented { + operation_name: "foo", +} +``` + +### The `Result` type alias should be defined in each module + +*Good*: + +* Reduces repetition +``` +pub type Result = std::result::Result; +... +fn foo() -> Result { true } +``` + +*Bad* +``` +... +fn foo() -> Result { true } +``` + + + +### `Err` variants should be returned with `fail()` + +*Good* +```rust +return NotImplemented { + operation_name: "Parquet format conversion", +}.fail(); +``` + +*Bad* +```rust +return Err(Error::NotImplemented { + operation_name: String::from("Parquet format conversion"), +}); +``` + + +### Use `context` to wrap underlying errors into module specific errors + +*Good*: + +* Reduces boilerplate + +```rust +input_reader + .read_to_string(&mut buf) + .context(UnableToReadInput { + input_filename, + })?; +``` + +*Bad* + +```rust +input_reader + .read_to_string(&mut buf) + .map_err(|e| Error::UnableToReadInput { + name: String::from(input_filename), + source: e, + })?; +``` + +### Each error cause in a module should have a distinct Error enum + +Specific error types are preferred over a generic error with a `message` or `kind` field. + +*Good*: + +- Makes it easier to track down the offending code based on a specific failure +- Reduces the size of the error enum (`String` is 3x 64-bit vs no space) +- Makes it easier to remove vestigial errors +- Is more concise + +```rust +#[derive(Debug, Snafu)] +pub enum Error { + #[snafu(display("Error writing remaining lines {}", source))] + UnableToWriteGoodLines { source: IngestError }, + + #[snafu(display("Error while closing the table writer {}", source))] + UnableToCloseTableWriter { source: IngestError }, +} + +// ... + +write_lines.context(UnableToWriteGoodLines)?; +close_writer.context(UnableToCloseTableWriter))?; +``` + + +*Bad* + +```rust +pub enum Error { + #[snafu(display("Error {}: {}", message, source))] + WritingError { + source: IngestError , + message: String, + }, +} + +write_lines.context(WritingError { + message: String::from("Error while writing remaining lines"), +})?; +close_writer.context(WritingError { + message: String::from("Error while closing the table writer"), +})?; +```