From f612b1a040a4f980477aef2584a9ab8ce1cb5cb1 Mon Sep 17 00:00:00 2001 From: Matt Enlow Date: Wed, 12 Jun 2024 10:56:47 -0700 Subject: [PATCH] manifestoed again dangit :( --- docs/control-flow_macros.cheatmd | 1 + docs/credo.cheatmd | 1 + docs/mix_configs.cheatmd | 14 ++ docs/module_directives.cheatmd | 47 +++++++ docs/styles.cheatmd | 219 ++++++++++++------------------- 5 files changed, 146 insertions(+), 136 deletions(-) create mode 100644 docs/control-flow_macros.cheatmd create mode 100644 docs/mix_configs.cheatmd create mode 100644 docs/module_directives.cheatmd diff --git a/docs/control-flow_macros.cheatmd b/docs/control-flow_macros.cheatmd new file mode 100644 index 0000000..a308cf3 --- /dev/null +++ b/docs/control-flow_macros.cheatmd @@ -0,0 +1 @@ +## Module Directives (`use`, `import`, `alias`, `require`, ...) diff --git a/docs/credo.cheatmd b/docs/credo.cheatmd index 0b1597e..6a89b10 100644 --- a/docs/credo.cheatmd +++ b/docs/credo.cheatmd @@ -1,6 +1,7 @@ ### Credo Rules Styler Replaces If you're using Credo and Styler, **we recommend disabling these rules in `.credo.exs`** to save on unnecessary checks in CI. +As long as you're running `mix format --check-formatted` in CI, Styler will be enforcing the rules for you, so checking them with Credo is redundant. Disabling the rules means updating your `.credo.exs` depending on your configuration: diff --git a/docs/mix_configs.cheatmd b/docs/mix_configs.cheatmd new file mode 100644 index 0000000..2ea3790 --- /dev/null +++ b/docs/mix_configs.cheatmd @@ -0,0 +1,14 @@ +## Mix Configs + +Mix Config files have their config stanzas sorted. Similar to the sorting of aliases, this delivers consistency to an otherwise arbitrary world, and can even help catch bugs like configuring the same key multiple times. + +A file is considered a config file if + +1. its path matches `config/.*\.exs` or `rel/overlays/.*\.exs` +2. the file imports Mix.Config (`import Mix.Config`) + +Once a file is detected as a mix config, its `config/2,3` stanzas are grouped and ordered like so: + +- group config stanzas separated by assignments (`x = y`) together +- sort each group according to erlang term sorting +- move all existing assignments between the config stanzas to above the stanzas (without changing their ordering) diff --git a/docs/module_directives.cheatmd b/docs/module_directives.cheatmd new file mode 100644 index 0000000..269a124 --- /dev/null +++ b/docs/module_directives.cheatmd @@ -0,0 +1,47 @@ +# Control Flow Macros (aka "Blocks": `case`, `if`, `unless`, `cond`, `with`) + +Elixir's Kernel documentation refers to these structures as "macros for control-flow". +We often refer to them as "blocks" in our changelog, which is a much worse name, to be sure. + +You're likely here just to see what Styler does, in which case, please [click here to skip](skip) following manifesto on our philosophy regarding the usage of these macros. + +## Which Control Flow Macro Should I Use? + +The number of "blocks" in Elixir means there are a large number of ways to write semantically equivalent code, often leaving developers [in the dark as to which structure they should use.](https://www.reddit.com/r/elixir/comments/1ctbtcl/i_am_completely_lost_when_it_comes_to_which/) + +We believe readability is enhanced by using the simplest api possible, whether we're talking about internal module function calls or standard-library macros. + +### `case`, `if`, & `unless` + +We advocate for `case` and `if` as the first tools to be considered for any control flow as they are the two simplest blocks. If a branch _can_ be expressed with an `if` statement, it _should_ be. Otherwise, `case` is the next best choice. In situations where developers might reach for an `if/elseif/else` block in other languages, `cond do` should be used. + +(`cond do` seems to see a paucity of use in the language, but many complex nested expressions or with statements can be improved by replacing them with a `cond do`). + +`unless` is a special case of `if` meant to make code read as natural-language (citation needed). While it sometimes succeeds in this goal, its absence in most programming languages often makes it feel cumbersome to programmers with non-Ruby backgrounds. Thankfully, with Styler's help developers don't need to ever reach for `unless` - expressions that are "simpler" with its use are automatically rewritten to use it. + +### `with` + +> `with` great power comes great responsibility +> +> - Uncle Ben + +As the most powerful of the Kernel control-flow expressions, `with` requires the most cognitive overhead from readers to be understood. Its power means that any expression that can be expressed as a `case`/`if`/`cond` can _also_ be expressed as a `with` statement (especially with the liberal application of small private helper functions). + +Unfortunately, this has lead to a proliferation of `with` in codebases where simpler expressions would have sufficed, meaning a lot of Elixir code ends up being harder for readers to understand than it needs to be. + +Thus, `with` is the control-flow structure of last resort. We advocate that `with` **should only be used when more basic expressions do not suffice or become overly verbose**. To qualify "overly verbose", we subscribe to the [Chris Keathley school of thought](https://www.youtube.com/watch?v=l-8ghbdRB1w) that judicious nesting of control flow blocks within a function isn't evil and more-often-than-not is superior to spreading implementation over many small single-use functions. We'd even go so far as to suggest that cyclomatic complexity is an inexact measure of code quality, with more than a few false negatives and many false positives. + +`with` is a great way to unnest multiple `case` statements when every failure branch of those statements results in the same error. This is easily and succinctly expressed with `with`'s `else` block: `else (_ -> :error)`. As Keathley says though, [Avoid Else In With Blocks](https://keathley.io/blog/good-and-bad-elixir.html#avoid-else-in-with-blocks). Having multiple else clauses "means that the error conditions matter. Which means that you don’t want `with` at all. You want `case`." + +It's acceptable to use one-line `with` statements (eg `with {:ok, _} <- Repo.update(changeset), do: :ok`) to signify that other branches are uninteresting or unmodified by your code, but ultimately that can hide the possible returns of a function from the reader, making it more onerous to debug all possible branches of the code in their mental model of the function. In other words, ideally all function calls in a `with` statement head have obvious error types for the reader, leaving their omission in the code acceptable as the reader feels no need to investigate further. The example at the start of this paragraph with an `Ecto.Repo` call is a good example, as most developers in a codebase using Ecto are expected to be familiar with its basic API. + +Using `case` rather than `with` for branches with unusual failure types can help document code as well as save the reader time in tracking down types. For example, replacing the following with a `with` statement that only matched against the `{:ok, _}` tuple would hide from readers that an atypically-shaped 3-tuple is returned when things go wrong. + +```elixir +case some_http_call() do + {:ok, _response} -> :ok + {:error, http_error, response} -> {:error, http_error, response} +end +``` + +### Styler's Here To Help diff --git a/docs/styles.cheatmd b/docs/styles.cheatmd index 4017997..ea44a30 100644 --- a/docs/styles.cheatmd +++ b/docs/styles.cheatmd @@ -2,35 +2,17 @@ ## Simple (Single Node) Styles - Function Performance & Readability Optimizations Optimizing for either performance or readability, probably both! These apply to the piped versions as well - ### Strings to Sigils Rewrites strings with 4 or more escaped quotes to string sigils with an alternative delimiter. The delimiter will be one of `" ( { | [ ' < /`, chosen by which would require the fewest escapes, and otherwise preferred in the order listed. -#### Before - -```elixir -conn -|> put_resp_content_type("application/json") -|> send_resp(403, "{\"errors\":[\"Not Authorized\"]}") -|> halt() -``` - -#### After - -```elixir -conn -|> put_resp_content_type("application/json") -|> send_resp(403, ~s({"errors":["Not Authorized"]}))) -|> halt() -``` +* `"{\"errors\":[\"Not Authorized\"]}"` => `~s({"errors":["Not Authorized"]})` ### Large Base 10 Numbers @@ -40,178 +22,143 @@ Formatter already does this except it doesn't rewrite "typos" like `100_000_0`. If you're concerned that this breaks your team's formatting for things like "cents" (like "$100" being written as `100_00`), consider using a library made for denoting currencies rather than raw elixir integers. -#### Before - -```elixir -10000 -1_0_0_0_0 # Elixir's formatter is fine with this --543213 -123456789 -55333.22 --123456728.0001 -``` - -#### After - -```elixir -10_000 -10_000 --543_213 -123_456_789 -55_333.22 --123_456_728.0001 -``` +* `10000 ` => `10_000` +* `1_0_0_0_0` => `10_000` (elixir's formatter leaves the former as-is) +* `-543213 ` => `-543_213` +* `123456789 ` => `123_456_789` +* `55333.22 ` => `55_333.22` +* `-123456728.0001 ` => `-123_456_728.0001` ### `Enum.into(%{}/Map/Keyword/MapSet.new)` -> `X.new` -While these examples use `%{}`, the same behaviour occurs for `Keyword.new()`, `MapSet.new()` and the empty map `%{}`. +This is an improvement for the reader, who gets a more natural language expression: "make a new map from enum" vs "enumerate enum and collect its elements into a new map" -This is an improvement for the reader, who gets a more natural language expression: "make a new map from a" vs "take a and enumerate it into a new map" - -#### Before - -```elixir -Enum.into(a, %{}) -Enum.into(a, %{}, mapping_function) -``` - -#### After - -```elixir -Map.new(a) -Map.new(a, mapping_function) -``` - -- Enum.into(%{}/Map/Keyword/MapSet.new) -> X.new +* `Enum.into(a, %{})` => `Map.new(enum)` +* `Enum.into(enum, Map.new())` => `Map.new(enum)` +* `Enum.into(enum, Keyword.new())` => `Keyword.new(enum)` +* `Enum.into(enum, MapSet.new())` => `Keyword.new(enum)` +* `Enum.into(enum, %{}, fn x -> {x, x} end)` => `Map.new(enum, fn x -> {x, x} end)` ### Map/Keyword.merge w/ single key literal -> X.put -`Keyword.merge` and `Map.merge` called with a literal map or keyword argument with a single key are rewritten to the -equivalent `put`, a cognitively simpler function. +`Keyword.merge` and `Map.merge` called with a literal map or keyword argument with a single key are rewritten to the equivalent `put`, a cognitively simpler function. -#### Before -```elixir -foo |> Keyword.merge(%{just_one_key: the_value}) |> bar() -``` - -#### After -```elixir -foo |> Keyword.put(:just_one_key, the_value) |> bar() -``` +* `Keyword.merge(kw, [key: :value])` => `Keyword.put(kw, :key, :value)` +* `Map.merge(map, %{key: :value})` => `Map.put(map, :key, :value)` +* `Map.merge(map, %{key => value})` => `Map.put(map, key, value)` +* `map |> Map.merge(%{key: value}) |> foo()` => `map |> Map.put(:key, value) |> foo()` ### Map/Keyword.drop w/ single key -> X.delete In the same vein as the `merge` style above, `[Map|Keyword].drop/2` with a single key to drop are rewritten to use `delete/2` -#### Before -```elixir -Map.drop(foo, [key]) -``` -#### After -```elixir -Map.delete(foo, key) -``` +* `Map.drop(map, [key])` => `Map.delete(map, key)` +* `Keyword.drop(kw, [key])` => `Keyword.delete(kw, key)` -### `Enum.reverse(foo) ++ bar -> Enum.reverse(foo, bar)` +### `Enum.reverse/1` and concatenation -> `Enum.reverse/2` `Enum.reverse/2` optimizes a two-step reverse and concatenation into a single step. -#### Before -```elixir -Enum.reverse(foo) ++ bar - -baz -|> Enum.reverse() -|> Enum.concat(bop) - -``` -#### After -```elixir -Enum.reverse(foo, bar) - -Enum.reverse(baz, bop) -``` +* `Enum.reverse(foo) ++ bar` => `Enum.reverse(foo, bar)` +* `baz |> Enum.reverse() |> Enum.concat(bop)` => `Enum.reverse(baz, bop)` -### Timex.now/0 -> DateTime.utc_now/0 +### `Timex.now/0` ->` DateTime.utc_now/0` Timex certainly has its uses, but knowing what stdlib date/time struct is returned by `now/0` is a bit difficult! -We prefer calling the actual function rather than its rename in Timex, helping the reader by being more explicit. -#### Before -```elixir -Timex.now() -``` -#### After -```elixir -DateTime.utc_now() -``` +We prefer calling the actual function rather than its rename in Timex, helping the reader by being more explicit. +This also hews to our internal styleguide's "Don't make one-line helper functions" guidance. -### DateModule.compare(x, y) == :lt/:gt -> DateModule.before?/after? +### `DateModule.compare/2` -> `DateModule.[before?|after?]` Again, the goal is readability and maintainability. `before?/2` and `after?/2` were implemented long after `compare/2`, so it's not unusual that a codebase needs a lot of refactoring to be brought up to date with these new functions. That's where Styler comes in! +* `DateTime.compare(start, end_date) == :gt` => `DateTime.after?(start, end_date)` +* `DateTime.compare(start, end_date) == :lt` => `DateTime.before?(start, end_date)` +* The same is done for `DateTime|NaiveDateTime|Time|Date.compare/2` + +### Implicit Try + +Styler will rewrite functions whose entire body is a try/do to instead use the implicit try syntax, per Credo's `Credo.Check.Readability.PreferImplicitTry` + +The following example illustrates the most complex case, but Styler happily handles just basic try do/rescue bodies just as easily. + #### Before + ```elixir -if DateTime.compare(start, end) == :gt, - do: :error, - else: :ok +def foo() do + try do + uh_oh() + rescue + exception -> {:error, exception} + catch + :a_throw -> {:error, :threw!} + else + try_has_an_else_clause? -> {:did_you_know, try_has_an_else_clause?} + after + :done + end +end ``` + #### After + ```elixir -if DateTime.after?(start, end), - do: :error, - else: :ok +def foo() do + uh_oh() +rescue + exception -> {:error, exception} +catch + :a_throw -> {:error, :threw!} +else + try_has_an_else_clause? -> {:did_you_know, try_has_an_else_clause?} +after + :done +end ``` -### Code Readability +### Remove parenthesis from 0-arity function & macro definitions -- put matches on right -- `Credo.Check.Readability.PreferImplicitTry` +The author of the library disagrees with this style convention :) BUT, the wonderful thing about Styler is it lets you write code how _you_ want to, while normalizing it for reading for your entire team. The most important thing is not having to think about the style, and instead focus on what you're trying to achieve. -### Consistency - `def foo()` -> `def foo` +- `defp foo()` -> `defp foo` +- `defmacro foo()` -> `defmacro foo` +- `defmacrop foo()` -> `defmacrop foo` ### Elixir Deprecation Rewrites 1.15+ -- Logger.warn -> Logger.warning -- Path.safe_relative_to/2 => Path.safe_relative/2 -- Enum/String.slice/2 w/ ranges -> explicit steps -- ~R/my_regex/ -> ~r/my_regex/ -- Date.range/2 -> Date.range/3 when decreasing range -- IO.read/bin_read -> use `:eof` instead of `:all` +- `Logger.warn` -> `Logger.warning` +- `Path.safe_relative_to/2` => `Path.safe_relative/2` +- `~R/my_regex/` -> `~r/my_regex/` +- `Enum/String.slice/2` with decreasing ranges -> add explicit steps to the range +- `Date.range/2` with decreasing range -> `Date.range/3` +- for both of the above ranges, the rewrite can only be applied if a literal range is being passed as an argument +- `IO.read/bin_read` with `:all` option -> replace `:all` with `:eof` 1.16+ -- File.stream!(file, options, line_or_bytes) => File.stream!(file, line_or_bytes, options) +- `File.stream!(file, options, line_or_bytes)` => `File.stream!(file, line_or_bytes, options)` -### Function Definitions -- Shrink multi-line function defs -- Put assignments on the right - -## Module Directives (`use`, `import`, `alias`, `require`, ...) +### Code Readability -## Mix Configs +- put matches on right +- `Credo.Check.Readability.PreferImplicitTry` -Mix Config files have their config stanzas sorted. Similar to the sorting of aliases, this delivers consistency to an otherwise arbitrary world, and can even help catch bugs like configuring the same key multiple times. +### Function Definitions -A file is considered a config file if +- Shrink multi-line function defs +- Put assignments on the right -1. its path matches `config/.*\.exs` or `rel/overlays/.*\.exs` -2. the file imports Mix.Config (`import Mix.Config`) -Once a file is detected as a mix config, its `config/2,3` stanzas are grouped and ordered like so: -- group config stanzas separated by assignments (`x = y`) together -- sort each group according to erlang term sorting -- move all existing assignments between the config stanzas to above the stanzas (without changing their ordering) -## Control Flow Structures (aka "Blocks": `case`, `if`, `unless`, `cond`, `with`) ### `case`