diff --git a/CODE_STYLE.md b/CODE_STYLE.md index 503844c2..77803ce3 100644 --- a/CODE_STYLE.md +++ b/CODE_STYLE.md @@ -1,100 +1,103 @@ # 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: + ```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 +105,303 @@ 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 blurs the focus of code, making it unnecessarily longer. + +## 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 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: - +**Rationale:** +- Less polluted import blocks +- Uniformity + +## `&str` -> `String` conversion + +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 -impl<'a> AnswerCallbackQuery<'a> { - pub(crate) fn new(bot: &'a Bot, callback_query_id: C) -> Self - where -C: Into, { ... } +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; ``` -
-## 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). +**Rationale:** +- Reading order is important for new contributors +- Grouping by crate allows spotting unwanted dependencies easier +- Consistency -## 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. +## Import Style -## 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")`. +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:** +- 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:** +- 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: +```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 a 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 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: + +```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 the `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.