Skip to content

Commit

Permalink
manifestoed again dangit :(
Browse files Browse the repository at this point in the history
  • Loading branch information
novaugust committed Jun 12, 2024
1 parent c957655 commit f612b1a
Show file tree
Hide file tree
Showing 5 changed files with 146 additions and 136 deletions.
1 change: 1 addition & 0 deletions docs/control-flow_macros.cheatmd
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
## Module Directives (`use`, `import`, `alias`, `require`, ...)
1 change: 1 addition & 0 deletions docs/credo.cheatmd
Original file line number Diff line number Diff line change
@@ -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:

Expand Down
14 changes: 14 additions & 0 deletions docs/mix_configs.cheatmd
Original file line number Diff line number Diff line change
@@ -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)
47 changes: 47 additions & 0 deletions docs/module_directives.cheatmd
Original file line number Diff line number Diff line change
@@ -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
219 changes: 83 additions & 136 deletions docs/styles.cheatmd
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand All @@ -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`

Expand Down

0 comments on commit f612b1a

Please sign in to comment.