diff --git a/modules/contributor/pages/code-style-guide.adoc b/modules/contributor/pages/code-style-guide.adoc index de64d8a78..a4ff4eea5 100644 --- a/modules/contributor/pages/code-style-guide.adoc +++ b/modules/contributor/pages/code-style-guide.adoc @@ -1,5 +1,55 @@ = Source code style guide +== Cargo.toml + +Follow the https://doc.rust-lang.org/nightly/style-guide/cargo.html[official formatting conventions] for the `Cargo.toml` file. +This means: + +* Put the `[package]` section at the top of the file. +* Put the `name` and `version` keys in that order at the top of that section, followed by the remaining keys other than `description` in order (sort keys with https://www.gnu.org/software/coreutils/manual/html_node/Version-sort-overview.html[version-sort]; very similar to lexical sorting)), followed by the `description` at the end of that section. +* For other sections, sort keys with version-sort. + +[TIP.code-rule,caption=Examples of correct code for this rule] +==== + +[source,toml] +---- +[package] +name = "crate-name" +version = "0.0.1" +# ... otherwise alphabetically sorted here +# ... and then the description as the last key +description = "this crate does nothing" + +[dependencies] +# dependencies sorted alphabetically +a_dependency = "1.2.3" +another_dependency = "0.1.0" +yet_another_dependency = "0.2.0" +---- + +==== + +[WARNING.code-rule,caption=Examples of incorrect code for this rule] +==== + +[source,toml] +---- +[package] +description = "this crate does nothing" +name = "crate-name" +version = "0.0.1" + +[dependencies] +another_dependency = "0.1.0" +yet_another_dependency = "0.2.0" +a_dependency = "1.2.3" +---- + +==== + +NOTE: This formatting might be supported by `rustfmt` in the future, see the https://github.com/rust-lang/rustfmt/pull/5240[PR] here. + == Identifier names === Long versus abbreviated @@ -12,34 +62,34 @@ The shorter the scope the shorter the variable names, and the longer the functio The usage of well-known acronyms like CPU, TLS or OIDC are allowed. -[WARNING.code-rule,caption=Examples of incorrect code for this rule] +[TIP.code-rule,caption=Examples of correct code for this rule] ==== [source,rust] ---- -const ONE_H_IN_SECS: usize = 3600; +const ONE_HOUR_IN_SECONDS: usize = 3600; -let param = Some("foo"); -let buf = &[]; +let parameter = Some("foo"); +let buffer = &[]; -fn func(elems: Vec) {} +fn function(elements: Vec) {} + +for i in 0..5 {} ---- ==== -[TIP.code-rule,caption=Examples of correct code for this rule] +[WARNING.code-rule,caption=Examples of incorrect code for this rule] ==== [source,rust] ---- -const ONE_HOUR_IN_SECONDS: usize = 3600; - -let parameter = Some("foo"); -let buffer = &[]; +const ONE_H_IN_SECS: usize = 3600; -fn function(elements: Vec) {} +let param = Some("foo"); +let buf = &[]; -for i in 0..5 {} +fn func(elems: Vec) {} ---- ==== @@ -80,24 +130,24 @@ let tls_settings = TlsSettings {}; Optional function parameters and variables containing `Option` must not use any prefixes or suffixes to indicate the value is of type `Option`. This rule does not apply to function names like `Client::get_opt()`. -[WARNING.code-rule,caption=Examples of incorrect code for this rule] +[TIP.code-rule,caption=Examples of correct code for this rule] ==== [source,rust] ---- -let tls_settings_or_none: Option = None; -let maybe_tls_settings: Option = None; -let opt_tls_settings: Option = None; +let tls_settings: Option = None; ---- ==== -[TIP.code-rule,caption=Examples of correct code for this rule] +[WARNING.code-rule,caption=Examples of incorrect code for this rule] ==== [source,rust] ---- -let tls_settings: Option = None; +let tls_settings_or_none: Option = None; +let maybe_tls_settings: Option = None; +let opt_tls_settings: Option = None; ---- ==== @@ -109,17 +159,17 @@ let tls_settings: Option = None; Structs can use singular and plural names. Enums must use singular names, because only one variant is valid, e.g. `Error::NotFound` and not `Errors::NotFound`. -[WARNING.code-rule,caption=Examples of incorrect code for this rule] +[TIP.code-rule,caption=Examples of correct code for this rule] ==== [source,rust] ---- -enum Errors { +enum Error { NotFound, Timeout, } -enum Colors { +enum Color { Red, Green, Blue, @@ -128,17 +178,17 @@ enum Colors { ==== -[TIP.code-rule,caption=Examples of correct code for this rule] +[WARNING.code-rule,caption=Examples of incorrect code for this rule] ==== [source,rust] ---- -enum Error { +enum Errors { NotFound, Timeout, } -enum Color { +enum Colors { Red, Green, Blue, @@ -149,27 +199,26 @@ enum Color { === Formatting of struct fields and enum variants -We add newlines to struct fields and enum variants when they include additional information like documentation comments or attributes, because the variants can become difficult to read. +Add newlines to struct fields and enum variants when they include additional information like documentation comments or attributes, because the variants can become difficult to read. This is especially the case when fields include doc comments, attributes like `#[snafu()]`, and in case of enum variants, various embedded types. Enum variants and struct fields don't need to be separated when **no** additional information is attached to any of the variants or fields. -[WARNING.code-rule,caption=Examples of incorrect code for this rule] +[TIP.code-rule,caption=Examples of correct code for this rule] ==== [source,rust] ---- enum Color { Red, - Green, - Blue, } struct Foo { /// My doc comment for bar bar: usize, + /// My doc comment for baz baz: usize, } @@ -178,6 +227,7 @@ enum Error { /// Indicates that we failed to foo. #[snafu(display("failed to foo"))] Foo, + /// Indicates that we failed to bar. #[snafu(display("failed to bar"))] Bar, @@ -187,21 +237,22 @@ enum Error { ==== -[TIP.code-rule,caption=Examples of correct code for this rule] +[WARNING.code-rule,caption=Examples of incorrect code for this rule] ==== [source,rust] ---- enum Color { Red, + Green, + Blue, } struct Foo { /// My doc comment for bar bar: usize, - /// My doc comment for baz baz: usize, } @@ -210,7 +261,6 @@ enum Error { /// Indicates that we failed to foo. #[snafu(display("failed to foo"))] Foo, - /// Indicates that we failed to bar. #[snafu(display("failed to bar"))] Bar, @@ -229,38 +279,48 @@ Comments should only be added if they provide additional information not availab === Choice of error crate and usage -We use `snafu` for all error handling in library *and* application code because we want to provide as much context to the user as possible. +Use `snafu` for all error handling in library *and* application code to provide as much context to the user as possible. Further, `snafu` allows us to use the same source error in multiple error variants. -This feature can be used for cases were we need / require more fine-grained error variants. +This feature can be used for cases where more fine-grained error variants are required. This behaviour is not possible when using `thiserror`, as it uses the `From` trait to convert the source error into an error variant. -Additionally, we restrict the usage of the `#[snafu(context(false))]` atrribute on error variants. +Additionally, the usage of the `#[snafu(context(false))]` atrribute on error variants is restricted. This ensures that fallible functions need to call `.context()` to pass the error along. The usage of `thiserror` is considered invalid. -[WARNING.code-rule,caption=Examples of incorrect code for this rule] +[TIP.code-rule,caption=Examples of correct code for this rule] ==== [source,rust] ---- -#[derive(thiserror::Error)] +#[derive(Snafu)] enum Error { - #[error("failed to read config file")] - FileRead(#[from] std::io::Error) + #[snafu(display("failed to read config file of user {user_name}"))] + FileRead { + source: std::io::Error, + user_name: String, + } } fn config_file(user: User) -> Result<(), Error> { - std::fs::read_to_string(user.file_path)?; + std::fs::read_to_string(user.file_path).context(FileReadSnafu { + user_name: user.name, + }); } ---- +==== + +[WARNING.code-rule,caption=Examples of incorrect code for this rule] +==== + [source,rust] ---- -#[derive(Snafu)] +#[derive(thiserror::Error)] enum Error { - #[snafu(context(false))] - FileRead { source: std::io::Error } + #[error("failed to read config file")] + FileRead(#[from] std::io::Error) } fn config_file(user: User) -> Result<(), Error> { @@ -268,26 +328,16 @@ fn config_file(user: User) -> Result<(), Error> { } ---- -==== - -[TIP.code-rule,caption=Examples of correct code for this rule] -==== - [source,rust] ---- #[derive(Snafu)] enum Error { - #[snafu(display("failed to read config file of user {user_name}"))] - FileRead { - source: std::io::Error, - user_name: String, - } + #[snafu(context(false))] + FileRead { source: std::io::Error } } fn config_file(user: User) -> Result<(), Error> { - std::fs::read_to_string(user.file_path).context(FileReadSnafu { - user_name: user.name, - }); + std::fs::read_to_string(user.file_path)?; } ---- @@ -300,31 +350,31 @@ Examples of such prefixes include (but are not limited to) `FailedTo` and `Unabl Furthermore, examples for suffixes are `Error` or `Snafu` Error variant names must however include verbs or identifiers as a prefix. -[WARNING.code-rule,caption=Examples of incorrect code for this rule] +[TIP.code-rule,caption=Examples of correct code for this rule] ==== [source,rust] ---- #[derive(Snafu)] enum Error { - FailedToParseConfig, - HttpRequestError, - ConfigRead, + ParseConfig, + HttpRequest, + ReadConfig, } ---- ==== -[TIP.code-rule,caption=Examples of correct code for this rule] +[WARNING.code-rule,caption=Examples of incorrect code for this rule] ==== [source,rust] ---- #[derive(Snafu)] enum Error { - ParseConfig, - HttpRequest, - ReadConfig, + FailedToParseConfig, + HttpRequestError, + ConfigRead, } ---- @@ -335,38 +385,38 @@ enum Error { All our error messages must start with a lowercase letter and must not end with a dot. It is recommended to start the error messages with "failed to..." or "unable to ...". -[WARNING.code-rule,caption=Examples of incorrect code for this rule] +[TIP.code-rule,caption=Examples of correct code for this rule] ==== [source,rust] ---- #[derive(Snafu)] enum Error { - #[snafu(display("Foo happened."))] + #[snafu(display("failed to foo"))] Foo, - #[snafu(display("Bar encountered"))] + #[snafu(display("unable to bar"))] Bar, - - #[snafu(display("arghh baz."))] - Baz, } ---- ==== -[TIP.code-rule,caption=Examples of correct code for this rule] +[WARNING.code-rule,caption=Examples of incorrect code for this rule] ==== [source,rust] ---- #[derive(Snafu)] enum Error { - #[snafu(display("failed to foo"))] + #[snafu(display("Foo happened."))] Foo, - #[snafu(display("unable to bar"))] + #[snafu(display("Bar encountered"))] Bar, + + #[snafu(display("arghh baz."))] + Baz, } ---- @@ -375,20 +425,20 @@ enum Error { ==== Examples for "failed to ..." error messages . `failed to parse config file` to indicate the parsing of the config file failed, usually because the file doesn't conform to the configuration language. -. `failed to construct http client` to indicate we wanted to construct a HTTP client to retrieve remote content. +. `failed to construct http client` to indicate that the construction of a HTTP client to retrieve remote content failed. ==== Exampled for "unable to ..." error messages -. `unable to read config file from ...` to indicate we could load the file (for example because the file doesn't exist). -. `unable to parse value ...` to indicate we failed to parse a user provided value which didn't conform to the expected syntax. +. `unable to read config file from ...` to indicate that the file could not be loaded (for example because the file doesn't exist). +. `unable to parse value ...` to indicate that parsing a user provided value failed (for example because it didn't conform to the expected syntax). == String formatting === Named versus unnamed format string identifiers -For simple string formatting (up to two substitutions), we allow unnamed (and thus also uncaptured) identifiers. +For simple string formatting (up to two substitutions), unnamed (and thus also uncaptured) identifiers are allowed. -For more complex formatting (more than two substitutions), we require named identifiers to avoid ambiguity, and to decouple argument order from the text (which can lead to incorrect text when the wording is changed and `{}` are reordered while the arguments aren't). +For more complex formatting (more than two substitutions), named identifiers are required to avoid ambiguity, and to decouple argument order from the text (which can lead to incorrect text when the wording is changed and `{}` are reordered while the arguments aren't). This rule needs to strike a balance between explicitness and concise `format!()` invocations. Long `format!()` expressions can lead to rustfmt breakage. It might be better to split up long formatting strings into multiple smaller ones. @@ -396,41 +446,41 @@ It might be better to split up long formatting strings into multiple smaller one Mix-and-matching of named versus unnamed identifiers must be avoided. See the next section about captured versus uncaptured identifiers. -[WARNING.code-rule,caption=Examples of incorrect code for this rule] +[TIP.code-rule,caption=Examples of correct code for this rule] ==== [source,rust] ---- format!( - "My {} {} string with {} substitutions is {}!", - "super", - "long", - 4, - "crazy", -); - -format!( - "My {quantifier} {} string with {count} substitutions is {}!", + "My {quantifier} {adjective} string with {count} substitutions is {description}!", quantifier = "super", - "long", + adjective = "long", count = 4, - "crazy", + description = "crazy", ); ---- ==== -[TIP.code-rule,caption=Examples of correct code for this rule] +[WARNING.code-rule,caption=Examples of incorrect code for this rule] ==== [source,rust] ---- format!( - "My {quantifier} {adjective} string with {count} substitutions is {description}!", + "My {} {} string with {} substitutions is {}!", + "super", + "long", + 4, + "crazy", +); + +format!( + "My {quantifier} {} string with {count} substitutions is {}!", quantifier = "super", - adjective = "long", + "long", count = 4, - description = "crazy", + "crazy", ); ---- @@ -438,7 +488,7 @@ format!( === Captured versus uncaptured format string identifiers -We place no restriction on named format string identifiers. +There are no restrictions on named format string identifiers. All options below are considered valid. [source,rust] @@ -454,10 +504,23 @@ format!("Hello {name}, hello again {name}", name = greetee); == Specifying resources measured in bytes and CPU fractions -We follow the Kubernetes convention described https://kubernetes.io/docs/reference/kubernetes-api/common-definitions/quantity/[here]. +Follow the Kubernetes convention described https://kubernetes.io/docs/reference/kubernetes-api/common-definitions/quantity/[here]. === Resources measured in bytes +[TIP.code-rule,caption=Examples of correct code for this rule] +==== + +[source,rust] +---- +let memory: MemoryQuantity = "100Mi".parse(); +let memory: MemoryQuantity = "1Gi".parse(); +let memory: MemoryQuantity = "1536Mi".parse(); +let memory: MemoryQuantity = "10Gi".parse(); +---- + +==== + [WARNING.code-rule,caption=Examples of incorrect code for this rule] ==== @@ -481,21 +544,21 @@ let memory: MemoryQuantity = "12345678".parse(); ==== +=== Resources measured in CPU fractions + [TIP.code-rule,caption=Examples of correct code for this rule] ==== [source,rust] ---- -let memory: MemoryQuantity = "100Mi".parse(); -let memory: MemoryQuantity = "1Gi".parse(); -let memory: MemoryQuantity = "1536Mi".parse(); -let memory: MemoryQuantity = "10Gi".parse(); +let memory: CpuQuantity = "100m".parse(); +let memory: CpuQuantity = "500m".parse(); +let memory: CpuQuantity = "1".parse(); +let memory: CpuQuantity = "2".parse(); ---- ==== -=== Resources measured in CPU fractions - [WARNING.code-rule,caption=Examples of incorrect code for this rule] ==== @@ -515,16 +578,3 @@ let memory: CpuQuantity = "2".parse(); ---- ==== - -[TIP.code-rule,caption=Examples of correct code for this rule] -==== - -[source,rust] ----- -let memory: CpuQuantity = "100m".parse(); -let memory: CpuQuantity = "500m".parse(); -let memory: CpuQuantity = "1".parse(); -let memory: CpuQuantity = "2".parse(); ----- - -====