diff --git a/modules/contributor/nav.adoc b/modules/contributor/nav.adoc index 6d3deb326..ad1d3f474 100644 --- a/modules/contributor/nav.adoc +++ b/modules/contributor/nav.adoc @@ -3,7 +3,8 @@ ** xref:steps.adoc[] ** xref:testing_on_kubernetes.adoc[] ** xref:development_dashboard.adoc[] -** xref:style_guide.adoc[] +** xref:code-style-guide.adoc[] +** xref:docs-style-guide.adoc[] ** Implementation guidelines *** xref:logging.adoc[] *** xref:service_discovery.adoc[] diff --git a/modules/contributor/pages/code-style-guide.adoc b/modules/contributor/pages/code-style-guide.adoc new file mode 100644 index 000000000..1927e5b2a --- /dev/null +++ b/modules/contributor/pages/code-style-guide.adoc @@ -0,0 +1,493 @@ += Source code style guide + +== Identifier names + +=== Long versus abbreviated + +We use unabbreviated identifier names to avoid ambiguity. +Short (even single letter) variable names are allowed in lambdas (closures), in one-liners, and when the context allows it. + +[quote,Uncle Bob Martin, 'Source: https://twitter.com/unclebobmartin/status/360029878126514177[Twitter]'] +The shorter the scope the shorter the variable names, and the longer the function [...] names. And vice versa. + +The usage of well-known acronyms like CPU, TLS or OIDC are allowed. + +[WARNING.code-rule,caption=Examples of incorrect code for this rule] +==== + +[source,rust] +---- +const ONE_H_IN_SECS: usize = 3600; + +let param = Some("foo"); +let buf = &[]; + +fn func(elems: Vec) {} +---- + +==== + +[TIP.code-rule,caption=Examples of correct code for this rule] +==== + +[source,rust] +---- +const ONE_HOUR_IN_SECONDS: usize = 3600; + +let parameter = Some("foo"); +let buffer = &[]; + +fn function(elements: Vec) {} + +for i in 0..5 {} +---- + +==== + +==== Closures and one-liners + +[NOTE] +==== +It should be noted that the second example is meant to illustrate the use of single letter variable names in closures. +It does *not* reflect production-level Rust code. +The snippet would be simplified in the real world. +==== + +[source,rust] +---- +let length = parameter.map(|p| p.len()); + +let sum: usize = vec![Some(2), None, Some(4), Some(3), None] + .iter() + .filter(|o| o.is_some()) + .map(|n| n.unwrap()) + .map(|n| n * 2) + .sum() +---- + +==== Well-known acronyms + +[source,rust] +---- +const K8S_LABEL_KEY: &str = "app.kubernetes.io"; + +let oidc_provider = OidcProvider {}; +let tls_settings = TlsSettings {}; +---- + +=== Optional function parameters and variables + +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] +==== + +[source,rust] +---- +let tls_settings_or_none: Option = None; +let maybe_tls_settings: Option = None; +let opt_tls_settings: Option = None; +---- + +==== + +[TIP.code-rule,caption=Examples of correct code for this rule] +==== + +[source,rust] +---- +let tls_settings: Option = None; +---- + +==== + +== Structs and enums + +=== Naming convention + +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] +==== + +[source,rust] +---- +enum Errors { + NotFound, + Timeout, +} + +enum Colors { + Red, + Green, + Blue, +} +---- + +==== + +[TIP.code-rule,caption=Examples of correct code for this rule] +==== + +[source,rust] +---- +enum Error { + NotFound, + Timeout, +} + +enum Color { + Red, + Green, + Blue, +} +---- + +==== + +=== 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. +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] +==== + +[source,rust] +---- +enum Color { + Red, + + Green, + + Blue, +} + +struct Foo { + /// My doc comment for bar + bar: usize, + /// My doc comment for baz + baz: usize, +} + +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, + Baz, +} +---- + +==== + +[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, +} + +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, + Baz, +} +---- + +==== + +Any single uncommented variants or fields in an otherwise-commented enum or struct is considered to be a smell. +If any of the items is commented, all items should be. +It should however also be noted that there is no requirement to comment fields or variants. +Comments should only be added if they provide additional information not available from context. + +== Error handling + +=== 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. +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 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. +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] +==== + +[source,rust] +---- +#[derive(thiserror::Error)] +enum Error { + #[error("failed to read config file")] + FileRead(#[from] std::io::Error) +} + +fn config_file(user: User) -> Result<(), Error> { + std::fs::read_to_string(user.file_path)?; +} +---- + +[source,rust] +---- +#[derive(Snafu)] +enum Error { + #[snafu(context(false))] + FileRead { source: std::io::Error } +} + +fn config_file(user: User) -> Result<(), Error> { + std::fs::read_to_string(user.file_path)?; +} +---- + +==== + +[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, + } +} + +fn config_file(user: User) -> Result<(), Error> { + std::fs::read_to_string(user.file_path).context(FileReadSnafu { + user_name: user.name, + }); +} +---- + +==== + +=== Error messages + +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] +==== + +[source,rust] +---- +#[derive(Snafu)] +enum Error { + #[snafu(display("Foo happened."))] + Foo, + + #[snafu(display("Bar encountered"))] + Bar, + + #[snafu(display("arghh baz."))] + Baz, +} +---- + +==== + +[TIP.code-rule,caption=Examples of correct code for this rule] +==== + +[source,rust] +---- +#[derive(Snafu)] +enum Error { + #[snafu(display("failed to foo"))] + Foo, + + #[snafu(display("unable to bar"))] + Bar, +} +---- + +==== + +==== 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. + +==== 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. + +== 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 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). +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. + +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] +==== + +[source,rust] +---- +format!( + "My {} {} string with {} substitutions is {}!", + "super", + "long", + 4, + "crazy", +); + +format!( + "My {quantifier} {} string with {count} substitutions is {}!", + quantifier = "super", + "long", + count = 4, + "crazy", +); +---- + +==== + +[TIP.code-rule,caption=Examples of correct code for this rule] +==== + +[source,rust] +---- +format!( + "My {quantifier} {adjective} string with {count} substitutions is {description}!", + quantifier = "super", + adjective = "long", + count = 4, + description = "crazy", +); +---- + +==== + +=== Captured versus uncaptured format string identifiers + +We place no restriction on named format string identifiers. +All options below are considered valid. + +[source,rust] +---- +let greetee = "world"; + +format!("Hello, {greetee}!"); +format!("Hello, {greetee}!", greetee = "universe"); +format!("Hello {name}, hello again {name}", name = greetee); +---- + +// TODO: Do we allow mix-and-matching captured and named identifiers? + +== 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]. + +=== Resources measured in bytes + +[WARNING.code-rule,caption=Examples of incorrect code for this rule] +==== + +[source,rust] +---- +// Biggest matching unit +let memory: MemoryQuantity = "100Mi".parse(); +let memory: MemoryQuantity = "1Gi".parse(); +let memory: MemoryQuantity = "1.5Gi".parse(); +let memory: MemoryQuantity = "10Gi".parse(); + +// Always Mi +let memory: MemoryQuantity = "100Mi".parse(); +let memory: MemoryQuantity = "1024Mi".parse(); +let memory: MemoryQuantity = "1536Mi".parse(); +let memory: MemoryQuantity = "10240Mi".parse(); + +// No unit at all +let memory: MemoryQuantity = "12345678".parse(); +---- + +==== + +[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(); +---- + +==== + +=== Resources measured in CPU fractions + +[WARNING.code-rule,caption=Examples of incorrect code for this rule] +==== + +[source,rust] +---- +// Always m +let memory: CpuQuantity = "100m".parse(); +let memory: CpuQuantity = "500m".parse(); +let memory: CpuQuantity = "1000m".parse(); +let memory: CpuQuantity = "2000m".parse(); + +// Floating points +let memory: CpuQuantity = "0.1".parse(); +let memory: CpuQuantity = "0.5".parse(); +let memory: CpuQuantity = "1".parse(); +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(); +---- + +==== diff --git a/modules/contributor/pages/style-guide.adoc b/modules/contributor/pages/docs-style-guide.adoc similarity index 99% rename from modules/contributor/pages/style-guide.adoc rename to modules/contributor/pages/docs-style-guide.adoc index f9eca4cf8..48eb8381f 100644 --- a/modules/contributor/pages/style-guide.adoc +++ b/modules/contributor/pages/docs-style-guide.adoc @@ -1,5 +1,5 @@ = Documentation style guide -:page-aliases: style_guide.adoc +:page-aliases: style_guide.adoc, style-guide.adoc :asciidoc-recommended-practices: https://asciidoctor.org/docs/asciidoc-recommended-practices[AsciiDoc recommended practices] :kubernetes-style-guide: https://kubernetes.io/docs/contribute/style/style-guide/[Kubernetes style guide] diff --git a/modules/contributor/pages/steps.adoc b/modules/contributor/pages/steps.adoc index e3137f244..af8fc3b42 100644 --- a/modules/contributor/pages/steps.adoc +++ b/modules/contributor/pages/steps.adoc @@ -21,7 +21,8 @@ tests need not to be adapted. Please skip the steps which are not applicable. == Changes in Rust projects . Make your desired changes in the according repository and test them manually. Ensure that the code compiles without - warnings (`cargo clippy --all-targets`) and that the code is formatted with `cargo fmt`. + warnings (`cargo clippy --all-targets`) and that the code is formatted with `cargo fmt`. Also make sure that all + changes are made in accordance to the xref:code-style-guide.adoc[source code style guide]. . If code was added or adapted then please create or adapt the unit tests in the same file as well as the integration tests in the `tests` directory. Ensure that all unit tests run successfully `cargo test`) and all integration tests run successfully (`./scripts/run_tests.sh`). See also <<_changes_in_the_integration_tests>>. @@ -93,7 +94,7 @@ helm install deploy/helm// repositories. Follow the steps in <> to be able to change the documentation. . Make your changes. . Build the documentation locally to ensure that the formatting is fine and all links are specified correctly. See the - {docs-readme}[`README.adoc`] file for further details and the xref:style_guide.adoc[] for style and formatting + {docs-readme}[`README.adoc`] file for further details and the xref:docs-style-guide.adoc[] for style and formatting guidelines. == Changes in the operator-templating diff --git a/ui b/ui index 5a393581e..b20d3d313 160000 --- a/ui +++ b/ui @@ -1 +1 @@ -Subproject commit 5a393581ef3d0e0c530705376078f4b5f06c5208 +Subproject commit b20d3d313854f24e85174319e559a7e82276a231