Skip to content

Latest commit

 

History

History
302 lines (220 loc) · 7.66 KB

style.md

File metadata and controls

302 lines (220 loc) · 7.66 KB
Raw
  • All of the codes should follow the Style Guidelines
  • All of the warning generated by Clippy should be fixed

Naming

Do not use usize if you only want a number, use it in memory index only

Do not use usize/u32/u64 directly in func parameter, wrap it

Good:

struct QueryId(Uuid);
fn get_query_id() -> QueryId;

Bad:

fn get_query_id(&self) -> Uuid;

Errors

All errors should follow the SNAFU crate philosophy and use SNAFU functionality

Good:

  • Derives Snafu and Debug functionality
  • Has a useful, end-user-friendly display message
#[derive(Snafu, Debug)]
pub enum Error {
    #[snafu(display(r#"Conversion needs at least one line of data"#))]
    NeedsAtLeastOneLine,
    // ...
}

Bad:

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
ensure!(!self.schema_sample.is_empty(), NeedsAtLeastOneLine);

Bad:

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
#[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:

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<T, E = Error> = std::result::Result<T, E>;
...
fn foo() -> Result<bool> { true }

Bad:

...
fn foo() -> Result<bool, Error> { true }

Err variants should be returned with fail()

Good:

return NotImplemented {
    operation_name: "Parquet format conversion",
}.fail();

Bad:

return Err(Error::NotImplemented {
    operation_name: String::from("Parquet format conversion"),
});

Use context to wrap underlying errors into module specific errors

Good:

  • Reduces boilerplate
input_reader
    .read_to_string(&mut buf)
    .context(UnableToReadInput {
        input_filename,
    })?;

Bad:

input_reader
    .read_to_string(&mut buf)
    .map_err(|e| Error::UnableToReadInput {
        name: String::from(input_filename),
        source: e,
    })?;

Hint for Box<dyn::std::error::Error> in Snafu:

If your error contains a trait object (e.g. Box<dyn std::error::Error + Send + Sync>), in order to use context() you need to wrap the error in a Box:

#[derive(Debug, Snafu)]
pub enum Error {

    #[snafu(display("gRPC planner got error listing partition keys: {}", source))]
    ListingPartitions {
        source: Box<dyn std::error::Error + Send + Sync>,
    },
}

...

  // Wrap error in a box prior to calling context()
database
  .partition_keys()
  .await
  .map_err(|e| Box::new(e) as _)
  .context(ListingPartitions)?;

Note the as _ in the map_err call. Without it, you may get an error such as:

error[E0271]: type mismatch resolving `<ListingPartitions as IntoError<influxrpc::Error>>::Source == Box<<D as Database>::Error>`
  --> query/src/frontend/influxrpc.rs:63:14
   |
63 |             .context(ListingPartitions)?;
   |              ^^^^^^^ expected trait object `dyn snafu::Error`, found associated type
   |
   = note: expected struct `Box<(dyn snafu::Error + Send + Sync + 'static)>`
              found struct `Box<<D as Database>::Error>`
   = help: consider constraining the associated type `<D as Database>::Error` to `(dyn snafu::Error + Send + Sync + 'static)`
   = note: for more information, visit https://doc.rust-lang.org/book/ch19-03-advanced-traits.html

Each error cause in a module should have a distinct Error enum variant

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
#[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:

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"),
})?;

Test

Do not return Result in test function, let it panic or return Report instead

Good:

#[test]
fn wtf() -> snafu::Report<Error>{}

Bad:

#[test]
fn wtf() -> Result<(), Error>{}

Trait and Generic

  • When generic is used, write the document about what the generic parameter is.
  • Encouraged to consturct a trait system before writing code

Async

❗ Async code should never spend a long time without reaching an .await ❗

Async: What is blocking

Using Rustlang's Async Tokio Runtime for CPU-Bound Tasks

Use spawn_blocking instead of block_in_place

According to the website, spawn_blocking is recommended to use. ❗ but if it is a very hot part of your application, then you should benchmark to compare ❗

Panic

  • Bug should panic
  • If you have to use unwrap/expect, you should guarantee the code never enter the exception code. And do not use unwrap, use expect instead(except test). Message in expect should follow this style.

Unsafe

  • Unsafe code should contain comments with Safety section
  • Wrapper of the unsafe code that is safe should check the parameter and return Result

ProtoBuf

  • Request should ends with Req suffix
  • Response should ends with Response suffix
  • Service should ends with Service suffix

Thanks

  • Thanks to the influxdb_iox, error handling is forked from them