From 45c1324cb37d7a823e83acabd159696f72a1359d Mon Sep 17 00:00:00 2001 From: Maybe Waffle Date: Thu, 29 Sep 2022 19:01:03 +0400 Subject: [PATCH 1/5] Improve readability of `CODE_STYLE.md` Former-commit-id: 36f0226751b254e0c70f4f2c8153f09bc254e97f --- CODE_STYLE.md | 279 +++++++++++++++++++++++++++++--------------------- 1 file changed, 163 insertions(+), 116 deletions(-) diff --git a/CODE_STYLE.md b/CODE_STYLE.md index 503844c2..eb0eced3 100644 --- a/CODE_STYLE.md +++ b/CODE_STYLE.md @@ -1,100 +1,101 @@ # Code style -This is a description of a coding style that every contributor must follow. Please, read the whole document before you start pushing code. + +This is a description of a coding style that every contributor must follow. +Please, read the whole document before you start pushing code. ## Generics -Generics are always written with `where`. -Bad: +All trait bounds should be written in `where` ```rust - pub fn new, - T: Into, - P: Into, - E: Into> +// GOOD +pub fn new(user_id: i32, name: N, title: T, png_sticker: P, emojis: E) -> Self +where + N: Into, + T: Into, + P: Into, + E: Into, +{ ... } + +// BAD +pub fn new, + T: Into, + P: Into, + E: Into> (user_id: i32, name: N, title: T, png_sticker: P, emojis: E) -> Self { ... } ``` - -Good: - ```rust - pub fn new(user_id: i32, name: N, title: T, png_sticker: P, emojis: E) -> Self - where - N: Into, - T: Into, - P: Into, - E: Into { ... } +// GOOD +impl Trait for Wrap +where + T: Trait +{ ... } + +// BAD +impl Trait for Wrap { ... } ``` -## Comments - 1. Comments must describe what your code does and mustn't describe how your code does it and bla-bla-bla. Be sure that your comments follow the grammar, including punctuation, the first capital letter and so on. +**Rationale:** `where` clauses are easier to read when there are a lot of bounds, uniformity. -Bad: +## Documentation comments -```rust -/// this function make request to telegram -pub fn make_request(url: &str) -> String { ... } -``` +1. Documentation must describe what your code does and mustn't describe how your code does it and bla-bla-bla. +2. Be sure that your comments follow the grammar, including punctuation, the first capital letter and so on. + ```rust + // GOOD + /// This function makes a request to Telegram. + pub fn make_request(url: &str) -> String { ... } + + // BAD + /// this function make request to telegram + pub fn make_request(url: &str) -> String { ... } + ``` +3. Do not use ending punctuation in short list items (usually containing just one phrase or sentence). + ```md + + - Handle different kinds of Update + - Pass dependencies to handlers + - Disable a default Ctrl-C handling -Good: + + - Handle different kinds of Update. + - Pass dependencies to handlers. + - Disable a default Ctrl-C handling. -```rust -/// This function makes a request to Telegram. -pub fn make_request(url: &str) -> String { ... } -``` + + - Handle different kinds of Update; + - Pass dependencies to handlers; + - Disable a default Ctrl-C handling; + ``` +3. Link resources in your comments when possible, for example: + ```rust + /// Download a file from Telegram. + /// + /// `path` can be obtained from the [`Bot::get_file`]. + /// + /// To download into [`AsyncWrite`] (e.g. [`tokio::fs::File`]), see + /// [`Bot::download_file`]. + /// + /// [`Bot::get_file`]: crate::bot::Bot::get_file + /// [`AsyncWrite`]: tokio::io::AsyncWrite + /// [`tokio::fs::File`]: tokio::fs::File + /// [`Bot::download_file`]: crate::Bot::download_file + ``` - 2. Do not use ending punctuation in short list items (usually containing just one phrase or sentence). Bad: +## Use `Self` where possible -```md - - Handle different kinds of Update. - - Pass dependencies to handlers. - - Disable a default Ctrl-C handling. -``` - -Bad: - -```md - - Handle different kinds of Update; - - Pass dependencies to handlers; - - Disable a default Ctrl-C handling. -``` - -Good: - -```md - - Handle different kinds of Update - - Pass dependencies to handlers - - Disable a default Ctrl-C handling -``` - - 3. Link resources in your comments when possible: - -```rust -/// Download a file from Telegram. -/// -/// `path` can be obtained from the [`Bot::get_file`]. -/// -/// To download into [`AsyncWrite`] (e.g. [`tokio::fs::File`]), see -/// [`Bot::download_file`]. -/// -/// [`Bot::get_file`]: crate::bot::Bot::get_file -/// [`AsyncWrite`]: tokio::io::AsyncWrite -/// [`tokio::fs::File`]: tokio::fs::File -/// [`Bot::download_file`]: crate::Bot::download_file -#[cfg(feature = "unstable-stream")] -pub async fn download_file_stream( - &self, - path: &str, -) -> Result>, reqwest::Error> -{ - download_file_stream(&self.client, &self.token, path).await -} -``` - -## Use Self where possible -Bad: +When referring to the type for which block is implemented, prefer using `Self`, rather than the name of the type. ```rust impl ErrorKind { + // GOOD + fn print(&self) { + Self::Io => println!("Io"), + Self::Network => println!("Network"), + Self::Json => println!("Json"), + } + + // BAD fn print(&self) { ErrorKind::Io => println!("Io"), ErrorKind::Network => println!("Network"), @@ -102,50 +103,96 @@ impl ErrorKind { } } ``` - -Good: ```rust -impl ErrorKind { - fn print(&self) { - Self::Io => println!("Io"), - Self::Network => println!("Network"), - Self::Json => println!("Json"), +impl<'a> AnswerCallbackQuery<'a> { + // GOOD + fn new(bot: &'a Bot, callback_query_id: C) -> Self + where + C: Into, + { ... } + + // BAD + fn new(bot: &'a Bot, callback_query_id: C) -> AnswerCallbackQuery<'a> + where + C: Into, + { ... } +} +``` + +**Rationale:** `Self` is generally shorter and it's easier to copy-paste code or rename the type. + +## Avoid duplication in fields names + +```rust +struct Message { + // GOOD + #[serde(rename = "message_id")] + id: MessageId, + + // BAD + message_id: MessageId, +} +``` + +**Rationale:** duplication is unnecessary + +## Conventional generic names + +Use a generic parameter name `S` for streams, `Fut` for futures, `F` for functions (where possible). + +**Rationale:** uniformity. + +## Deriving traits + +Derive `Copy`, `Clone`, `Eq`, `PartialEq`, `Hash` and `Debug` for public types when possible. + +**Rationale:** these traits can be useful for users and can be implemented for most types. + +Derive `Default` when there is a reasonable default value for the type. + +**Rationale:** `Default` plays nicely with generic code (for example `mem::take`). + +## `Into`-polymorphism + +Use `T: Into` when this can simplify user code. +I.e. when there are types that implement `Into` that are likely to be passed to this function. + +**Rationale:** conversions unnecessarily complicate caller code and can be confusing for beginners. + +## `must_use` + +Always mark a functions as `#[must_use]` if they don't have side-effects and the only reason to call them is to get the result. + +```rust +impl User { + // GOOD + #[must_use] + fn full_name(&self) -> String { + format!("{} {}", user.first_name, user.last_name) } } ``` -
- More examples - -Bad: - +**Rationale:** users will get warnings if they forgot to do something with the result, potentially preventing bugs. + +## Creating boxed futures + +Prefer `Box::pin(async { ... })` instead of `async { ... }.boxed()`. + +**Rationale:** the former is generally formatted better by rustfmt. + +## Full paths for logging + +Always write `log::!(...)` instead of importing `use log::;` and invoking `!(...)`. + ```rust -impl<'a> AnswerCallbackQuery<'a> { - pub(crate) fn new(bot: &'a Bot, callback_query_id: C) -> AnswerCallbackQuery<'a> - where -C: Into, { ... } +// GOOD +log::warn!("Everything is on fire"); + +// BAD +use log::warn; + +warn!("Everything is on fire"); ``` -Good: - -```rust -impl<'a> AnswerCallbackQuery<'a> { - pub(crate) fn new(bot: &'a Bot, callback_query_id: C) -> Self - where -C: Into, { ... } -``` -
- -## Naming - 1. Avoid unnecessary duplication (`Message::message_id` -> `Message::id` using `#[serde(rename = "message_id")]`). - 2. Use a generic parameter name `S` for streams, `Fut` for futures, `F` for functions (where possible). - -## Deriving - 1. Derive `Copy`, `Eq`, `Hash`, `PartialEq`, `Clone`, `Debug` for public types when possible (note: if the default `Debug` implementation is weird, you should manually implement it by yourself). - 2. Derive `Default` when there is an algorithm to get a default value for your type. - -## Misc - 1. Use `Into<...>` only where there exists at least one conversion **and** it will be logically to use. - 2. Always mark a function as `#[must_use]` if its return value **must** be used. - 3. `Box::pin(async [move] { ... })` instead of `async [move] { ... }.boxed()`. - 4. Always write `log::!(...)` instead of importing `use log::;` and invoking `!(...)`. For example, write `log::info!("blah")`. +**Rationale:** uniformity, it's clearer which log crate is used. From bb4abf6d985761b702d2bc8ac62940678df60434 Mon Sep 17 00:00:00 2001 From: Maybe Waffle Date: Thu, 29 Sep 2022 19:05:31 +0400 Subject: [PATCH 2/5] Codify `&str` -> `String` conversions in code style Former-commit-id: 5e87accfc73ae15389738bb71caf51d6c39e536e --- CODE_STYLE.md | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/CODE_STYLE.md b/CODE_STYLE.md index eb0eced3..515ca25b 100644 --- a/CODE_STYLE.md +++ b/CODE_STYLE.md @@ -196,3 +196,9 @@ warn!("Everything is on fire"); ``` **Rationale:** uniformity, it's clearer which log crate is used. + +## `&str` -> `String` conversion + +Prefer using `.to_owned()`, rather than `.to_string()`, `.into()`, `String::from`, etc. + +**Rationale:** uniformity, intent clarity. From 35cc2d1b2b9953dfeb4a40c0393b5e6f398ff049 Mon Sep 17 00:00:00 2001 From: Maybe Waffle Date: Thu, 29 Sep 2022 19:17:16 +0400 Subject: [PATCH 3/5] COpy some code style rules from r-a Former-commit-id: 562f0479024bfc8299b01120d4927136e50a0df0 --- CODE_STYLE.md | 188 ++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 188 insertions(+) diff --git a/CODE_STYLE.md b/CODE_STYLE.md index 515ca25b..21534c95 100644 --- a/CODE_STYLE.md +++ b/CODE_STYLE.md @@ -202,3 +202,191 @@ warn!("Everything is on fire"); Prefer using `.to_owned()`, rather than `.to_string()`, `.into()`, `String::from`, etc. **Rationale:** uniformity, intent clarity. + +## Order of imports + +Separate import groups with blank lines. Use one use per crate. + +Module declarations come before the imports. +Order them in "suggested reading order" for a person new to the code base. + +```rust +mod x; +mod y; + +// First std. +use std::{ ... } + +// Second, external crates (both crates.io crates and other rust-analyzer crates). +use crate_foo::{ ... } +use crate_bar::{ ... } + +// Then current crate. +use crate::{} + +// Finally, parent and child modules, but prefer `use crate::`. +use super::{} + +// Re-exports are treated as item definitions rather than imports, so they go +// after imports and modules. Use them sparingly. +pub use crate::x::Z; +``` + +**Rationale:** consistency. Reading order is important for new contributors. Grouping by crate allows spotting unwanted dependencies easier. + +## Import Style + +When implementing traits from `std::fmt` import the module: + +```rust +// GOOD +use std::fmt; + +impl fmt::Display for RenameError { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { .. } +} + +// BAD +impl std::fmt::Display for RenameError { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { .. } +} +``` + +**Rationale:** overall, less typing. Makes it clear that a trait is implemented, rather than used. + +Prefer `use crate::foo::bar` to `use super::bar` or `use self::bar::baz`. **Rationale:** consistency, this is the style which works in all cases. + +## Order of Items + +Optimize for the reader who sees the file for the first time, and wants to get a general idea about what's going on. People read things from top to bottom, so place most important things first. + +Specifically, if all items except one are private, always put the non-private item on top. +```rust +// GOOD +pub(crate) fn frobnicate() { + Helper::act() +} + +#[derive(Default)] +struct Helper { stuff: i32 } + +impl Helper { + fn act(&self) { + + } +} + +// BAD +#[derive(Default)] +struct Helper { stuff: i32 } + +pub(crate) fn frobnicate() { + Helper::act() +} + +impl Helper { + fn act(&self) { + + } +} +``` + +If there's a mixture of private and public items, put public items first. + +Put structs and enums first, functions and impls last. Order type declarations in top-down manner. + +```rust +// GOOD +struct Parent { + children: Vec +} + +struct Child; + +impl Parent { +} + +impl Child { +} + +// BAD +struct Child; + +impl Child { +} + +struct Parent { + children: Vec +} + +impl Parent { +} +``` + +**Rationale:** easier to get the sense of the API by visually scanning the file. If function bodies are folded in the editor, the source code should read as documentation for the public API. + +## Early Returns + +Do use early returns + +```rust +// GOOD +fn foo() -> Option { + if !condition() { + return None; + } + + Some(...) +} + +// BAD +fn foo() -> Option { + if condition() { + Some(...) + } else { + None + } +} +``` + +**Rationale:** reduce cognitive stack usage. + +## If-let + +Avoid if let ... { } else { } construct, use match instead. + +```rust +// GOOD +match ctx.expected_type.as_ref() { + Some(expected_type) => completion_ty == expected_type && !expected_type.is_unit(), + None => false, +} + +// BAD +if let Some(expected_type) = ctx.expected_type.as_ref() { + completion_ty == expected_type && !expected_type.is_unit() +} else { + false +} +``` + +**Rationale:** `match is almost always more compact. The `else` branch can get a more precise pattern: `None` or `Err(_)` instead of `_`. + +## Empty Match Arms + +Use `=> (),` when a match arm is intentionally empty: +```rust +// GOOD +match result { + Ok(_) => (), + Err(err) => error!("{}", err), +} + +// BAD +match result { + Ok(_) => {} + Err(err) => error!("{}", err), +} +``` + +**Rationale:** consistency. From 72a1be63f6ce81753ff41b9f816384cb4a76e491 Mon Sep 17 00:00:00 2001 From: Waffle Maybe Date: Mon, 3 Oct 2022 13:48:14 +0400 Subject: [PATCH 4/5] Apply suggestions from code review Co-authored-by: Hirrolot Former-commit-id: 1b983e043f2d744bf5e971fe8d642f805ea26074 --- CODE_STYLE.md | 51 +++++++++++++++++++++++++++++++++------------------ 1 file changed, 33 insertions(+), 18 deletions(-) diff --git a/CODE_STYLE.md b/CODE_STYLE.md index 21534c95..67c3d42e 100644 --- a/CODE_STYLE.md +++ b/CODE_STYLE.md @@ -5,7 +5,7 @@ Please, read the whole document before you start pushing code. ## Generics -All trait bounds should be written in `where` +All trait bounds should be written in `where`: ```rust // GOOD @@ -35,11 +35,13 @@ where impl Trait for Wrap { ... } ``` -**Rationale:** `where` clauses are easier to read when there are a lot of bounds, uniformity. +**Rationale:** +- `where` clauses are easier to read when there are a lot of bounds +- uniformity ## Documentation comments -1. Documentation must describe what your code does and mustn't describe how your code does it and bla-bla-bla. +1. Documentation must describe _what_ your code does and mustn't describe _how_ your code does it and bla-bla-bla. 2. Be sure that your comments follow the grammar, including punctuation, the first capital letter and so on. ```rust // GOOD @@ -67,7 +69,7 @@ impl Trait for Wrap { ... } - Pass dependencies to handlers; - Disable a default Ctrl-C handling; ``` -3. Link resources in your comments when possible, for example: +3. Link resources in your comments when possible: ```rust /// Download a file from Telegram. /// @@ -84,7 +86,7 @@ impl Trait for Wrap { ... } ## Use `Self` where possible -When referring to the type for which block is implemented, prefer using `Self`, rather than the name of the type. +When referring to the type for which block is implemented, prefer using `Self`, rather than the name of the type: ```rust impl ErrorKind { @@ -134,7 +136,7 @@ struct Message { } ``` -**Rationale:** duplication is unnecessary +**Rationale:** duplication blurs the focus of code, making it unnecessarily longer. ## Conventional generic names @@ -150,7 +152,7 @@ Derive `Copy`, `Clone`, `Eq`, `PartialEq`, `Hash` and `Debug` for public types w Derive `Default` when there is a reasonable default value for the type. -**Rationale:** `Default` plays nicely with generic code (for example `mem::take`). +**Rationale:** `Default` plays nicely with generic code (for example, `mem::take`). ## `Into`-polymorphism @@ -161,7 +163,7 @@ I.e. when there are types that implement `Into` that are likely to be passed ## `must_use` -Always mark a functions as `#[must_use]` if they don't have side-effects and the only reason to call them is to get the result. +Always mark functions as `#[must_use]` if they don't have side effects and the only reason to call them is to get the result: ```rust impl User { @@ -195,7 +197,9 @@ use log::warn; warn!("Everything is on fire"); ``` -**Rationale:** uniformity, it's clearer which log crate is used. +**Rationale:** +- Less polluted import blocks +- Uniformity ## `&str` -> `String` conversion @@ -232,7 +236,10 @@ use super::{} pub use crate::x::Z; ``` -**Rationale:** consistency. Reading order is important for new contributors. Grouping by crate allows spotting unwanted dependencies easier. +**Rationale:** +- Reading order is important for new contributors +- Grouping by crate allows spotting unwanted dependencies easier +- Consistency ## Import Style @@ -252,15 +259,19 @@ impl std::fmt::Display for RenameError { } ``` -**Rationale:** overall, less typing. Makes it clear that a trait is implemented, rather than used. +**Rationale:** +- Makes it clear that a trait is implemented, rather than used +- Less typing -Prefer `use crate::foo::bar` to `use super::bar` or `use self::bar::baz`. **Rationale:** consistency, this is the style which works in all cases. +Prefer `use crate::foo::bar` to `use super::bar` or `use self::bar::baz`. **Rationale:** +- Works in all cases +- Consistency ## Order of Items Optimize for the reader who sees the file for the first time, and wants to get a general idea about what's going on. People read things from top to bottom, so place most important things first. -Specifically, if all items except one are private, always put the non-private item on top. +Specifically, if all items except one are private, always put the non-private item on top: ```rust // GOOD pub(crate) fn frobnicate() { @@ -293,7 +304,7 @@ impl Helper { If there's a mixture of private and public items, put public items first. -Put structs and enums first, functions and impls last. Order type declarations in top-down manner. +Put structs and enums first, functions and impls last. Order type declarations in a top-down manner: ```rust // GOOD @@ -323,11 +334,13 @@ impl Parent { } ``` -**Rationale:** easier to get the sense of the API by visually scanning the file. If function bodies are folded in the editor, the source code should read as documentation for the public API. +**Rationale:** +- Easier to get a sense of the API by visually scanning the file +- If function bodies are folded in the editor, the source code should be read as documentation for the public API ## Early Returns -Do use early returns +Do use early returns: ```rust // GOOD @@ -353,7 +366,7 @@ fn foo() -> Option { ## If-let -Avoid if let ... { } else { } construct, use match instead. +Avoid the `if let ... { } else { }` construct, use `match` instead: ```rust // GOOD @@ -370,7 +383,9 @@ if let Some(expected_type) = ctx.expected_type.as_ref() { } ``` -**Rationale:** `match is almost always more compact. The `else` branch can get a more precise pattern: `None` or `Err(_)` instead of `_`. +**Rationale:** +- `match` is almost always more compact +- The `else` branch can get a more precise pattern: `None` or `Err(_)` instead of `_` ## Empty Match Arms From 95bf58bff018052f9cd3b78a9f509743dcebb258 Mon Sep 17 00:00:00 2001 From: Waffle Maybe Date: Mon, 3 Oct 2022 20:49:25 +0400 Subject: [PATCH 5/5] Apply suggestions from code review Co-authored-by: Hirrolot Former-commit-id: 243bfa84608d0f70838658724d59a1238e8c6a28 --- CODE_STYLE.md | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/CODE_STYLE.md b/CODE_STYLE.md index 67c3d42e..77803ce3 100644 --- a/CODE_STYLE.md +++ b/CODE_STYLE.md @@ -42,7 +42,7 @@ impl Trait for Wrap { ... } ## Documentation comments 1. Documentation must describe _what_ your code does and mustn't describe _how_ your code does it and bla-bla-bla. -2. Be sure that your comments follow the grammar, including punctuation, the first capital letter and so on. +2. Be sure that your comments follow the grammar, including punctuation, the first capital letter and so on: ```rust // GOOD /// This function makes a request to Telegram. @@ -52,7 +52,7 @@ impl Trait for Wrap { ... } /// this function make request to telegram pub fn make_request(url: &str) -> String { ... } ``` -3. Do not use ending punctuation in short list items (usually containing just one phrase or sentence). +3. Do not use ending punctuation in short list items (usually containing just one phrase or sentence): ```md - Handle different kinds of Update