diff --git a/CHANGELOG.md b/CHANGELOG.md index 894e658..55f0f95 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,8 @@ # Changelog +**Note** Styler's only public API is its usage as a formatter plugin. While you're welcome to play with its internals, +they can and will change without that change being reflected in Styler's semantic version. + ## main ### Fixes diff --git a/README.md b/README.md index 6431660..a193297 100644 --- a/README.md +++ b/README.md @@ -5,11 +5,30 @@ you what's wrong, it just rewrites the code for you to fit its style rules. You can learn more about the history, purpose and implementation of Styler from our talk: [Styler: Elixir Style-Guide Enforcer @ GigCity Elixir 2023](https://www.youtube.com/watch?v=6pF8Hl5EuD4) ------------------------ +## Features -Styler's documentation is under work as part of releasing 1.0. +- auto-fixes [many credo rules](docs/credo.md), meaning you can turn them off to speed credo up +- [keeping a strict module layout](docs/module_directives.md#directive-organization) +- alphabetizing module directives +- [extracting repeated aliases](docs/moduledirectives.md#alias-lifting) +- piping and unpiping function calls based on the number of functons +- optimizing standard library calls (`a |> Enum.map(m) |> Enum.into(Map.new)` => `Map.new(a, m)`) +- using sigils for strings with many escaped quotes `\"` +- ... and so many other things. -You can find the much more complete and usable [0.11.9 documentation and readme here.](https://hexdocs.pm/styler/readme.html) +[See our Rewrites documentation on hexdocs for all the nitty-gritty on what all Styler does](https://hexdocs.pm/styler/) + +## Who is Styler for? + +Styler was designed for a large team (40+ engineers) working in a single codebase. It helps remove fiddly code review comments and removes failed linter CI slowdowns, helping teams get things done faster. Teams in similar situations might appreciate Styler. + +Its automations are also extremely valuable for taming legacy elixir codebases or just refactoring in general. Some of its rewrites have inspired code actions in elixir language servers. + +Conversely, Styler probably _isn't_ a good match for: + +- libraries +- experimental, macro-heavy codebases +- small teams that don't want to think about code standards ## Installation @@ -23,51 +42,70 @@ def deps do end ``` -Please excuse the mess below as I find spare time to update our documentation =) Anything with TODOs are, well, notes to myself on documentation that needs rewriting. Happy to accept PRs if one seems doable to others. - -_@TODO put this somewhere more reasonable_ - -**Note** Styler's only public API is its usage as a formatter plugin. While you're welcome to play with its internals, -they can and will change without that change being reflected in Styler's semantic version. - Then add `Styler` as a plugin to your `.formatter.exs` file ```elixir [ plugins: [Styler] - # optionally: include styler configuration - # , styler: [alias_lifting_excludes: []] ] ``` And that's it! Now when you run `mix format` you'll also get the benefits of Styler's Stylish Stylings. +**Speed**: Expect the first run to take some time as `Styler` rewrites violations of styles and bottlenecks on disk I/O. Subsequent formats formats won't take noticeably more time. + ### Configuration -@TODO document: config for lifting, and why we won't add options other configs +Styler can be configured in your `.formatter.exs` file + +```elixir +[ + plugins: [Styler], + styler: [ + alias_lifting_exclude: [...] + ] +] +``` -Styler is @adobe's internal Style Guide Enforcer - allowing exceptions to the styles goes against that ethos. Happily, it's open source and thus yours to do with as you will =) +Styler's only current configuration option is `:alias_lifting_exclude`, which accepts a list of atoms to _not_ lift. See the [Module Directive documentation](docs/module_directives.md#alias-lifting) for more. -## Features (or as we call them, "Styles") +#### No Credo-Style Enable/Disable -@TODO link examples +Styler [will not add configuration](https://github.com/adobe/elixir-styler/pull/127#issuecomment-1912242143) for ad-hoc enabling/disabling of rewrites. Sorry! Its implementation simply does not support that approach. There are however many forks out there that have attempted this; please explore the [Github forks tab](https://github.com/adobe/elixir-styler/forks) to see if there's a project that suits your needs or that you can draw inspiration from. -https://hexdocs.pm/styler/1.0.0-rc.0/styles.html +Ultimately Styler is @adobe's internal tool that we're happy to share with the world. We're delighted if you like it as is, and just as excited if it's a starting point for you to make something even better for yourself. -## Styler & Credo +## !Styler can change the behaviour of your program! -@TODO link credo doc +The best example of the way in which Styler changes the meaning of your code is the following rewrite: +```elixir +# Before: this case statement... +case foo do + true -> :ok + false -> :error +end -## Your first Styling +# After: ... is rewritten by Styler to be an if statement!. +if foo do + :ok +else + :error +end +``` -**Speed**: Expect the first run to take some time as `Styler` rewrites violations of styles. +These programs are not semantically equivalent. The former would raise if `foo` returned any value other than `true` or `false`, while the latter blissfully completes. -Once styled the first time, future styling formats shouldn't take noticeably more time. +However, Styler is about _style_, and the `if` statement is (in our opinion) of much better style. If the exception behaviour was intentional on the code author's part, they should have written the program like this: -## Styler can break your code +```elixir +case foo do + true -> :ok + false -> :error + other -> raise "expected `true` or `false`, got: #{inspect other}" +end +``` -@TODO link troubleshooting -mention our rewrite of case true false to if and how we're OK with this being _Styler_, not _SemanticallyEquivalentRewriter_. +Also good style! But Styler assumes that most of the time people just meant the `if` equivalent of the code, and so makes that change. If issues like this bother you, Styler probably isn't the tool you're looking for. ## Thanks & Inspiration diff --git a/docs/control_flow_macros.md b/docs/control_flow_macros.md new file mode 100644 index 0000000..d4123a9 --- /dev/null +++ b/docs/control_flow_macros.md @@ -0,0 +1,315 @@ +# Control Flow Macros (`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](#if-and-unless) the 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 many 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. + +### use `case`, `if`, or `unless` when... + +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. + +### use `with` when... + +> `with` great power comes great responsibility +> +> - Uncle Ben + + +As the most powerful of the Kernel control-flow expressions, `with` requires the most cognitive overhead to understand. Its power means that we can use it as a replacement for anything we might express using a `case`, `if`, or `cond` (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**. As for verbosity, 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 +``` + +## `if` and `unless` + +Styler removes `else: nil` clauses: + +```elixir +if a, do: b, else: nil +# styled: +if a, do: b +``` + +### Negation Inversion + +Styler removes negators in the head of `if` and `unless` statements by "inverting" the statement. +The following operators are considered "negators": `!`, `not`, `!=`, `!==` + + +Examples: + +```elixir +# negated `if` statement with no `else` clause are rewritten to `unless` +if not x, do: y +# Styled: +unless x, do: y + +# negated `if` statements with an `else` clause have their clauses inverted and negation removed +if !x, do: y, else: z +# Styled: +if x, do: z, else: y + +# negated `unless` statements are rewritten to `if` +unless x != y, do: z +# B styled: +if x == y, do: z + +# `unless` with `else` is verboten; these are always rewritten to `if` statements +unless x, do: y, else: z +# styled: +if x, do: z, else: y +``` + +Because elixir relies on truthy/falsey values for its `if` statements, boolean casting is unnecessary and so double negation is simply removed. + +```elixir +if !!x, do: y +# styled: +if x, do: y +``` + +## `case` + +### "Erlang heritage" `case` true/false -> `if` + +Trivial true/false `case` statements are rewritten to `if` statements. While this results in a [semantically different program](https://github.com/rrrene/credo/issues/564#issue-338349517), we argue that it results in a better program for maintainability. If the developer wants a their case statement to raise when receiving a non-boolean value as a feature of the program, they would better serve their callers by raising something more descriptive. + +In other words, Styler leaves the code with better style, trumping obscure exception design :) + +```elixir +# instead of this +case foo do + true -> :ok + false -> :error +end + +# do this +if foo do + :ok +else + :error +end + +# OR this. readers now know that the exception is an intentional design, +# rather than an accidental "feature" +case foo do + true -> :ok + false -> :error + other -> raise "expected `true` or `false`, got: #{inspect other}" +end +``` + +## `cond` + +Styler has only one `cond` statement rewrite: replace 2-clause statements with `if` statements. + +```elixir +# Given +cond do + a -> b + true -> c +end +# Styled +if a do + b +else + c +end +``` + +## `with` + +`with` statements are extremely expressive. Styler tries to remove any unnecessary complexity from them in the following ways. + +### Remove Identity Else Clause + +Like if statements with `nil` as their else clause, the identity `else` clause is the default for `with` statements and so is removed. + +```elixir +# Given +with :ok <- b(), :ok <- b() do + foo() +else + error -> error +end +# Styled: +with :ok <- b(), :ok <- b() do + foo() +end +``` + +### Remove The Statement Entirely + +While you might think "surely this kind of code never appears in the wild", it absolutely does. Typically it's the result of someone refactoring a pattern away and not looking at the larger picture and realizing that the with statement now serves no purpose. + +Maybe someday the compiler will warn about these use cases. Until then, Styler to the rescue. + +```elixir +# Given: +with a <- b(), + c <- d(), + e <- f(), + do: g, + else: (_ -> h) +# Styled: +a = b() +c = d() +e = f() +g + +# Given +with value <- arg do + value +end +# Styled: +arg +``` + +### Replace `_ <- rhs` with `rhs` + +This is another case of "less is more" for the reader. + +```elixir +# Given +with :ok <- x, + _ <- y(), + {:ok, _} <- z do + :ok +end +# Styled: +with :ok <- x, + y(), + {:ok, _} <- z do + :ok +end +``` + +### Replace non-branching `bar <-` with `bar =` + +`<-` is for branching. If the lefthand side is the trivial match (a bare variable), Styler rewrites it to use the `=` operator instead. + +```elixir +# Given +with :ok <- foo(), + bar <- baz(), + :ok <- woo(), + do: {:ok, bar} +# Styled + with :ok <- foo(), + bar = baz(), + :ok <- woo(), + do: {:ok, bar} +``` + +### Move assignments from `with` statement head + +Just because any program _could_ be written entirely within the head of a `with` statement doesn't mean it should be! + +Styler moves assignments that aren't trapped between `<-` outside of the head. Combined with the non-pattern-matching replacement above, we get the following: + +```elixir +# Given +with foo <- bar, + x = y, + :ok <- baz, + bop <- boop, + :ok <- blop, + foo <- bar, + :success = hope_this_works! do + :ok +end +# Styled: +foo = bar +x = y + +with :ok <- baz, + bop = boop, + :ok <- blop do + foo = bar + :success = hope_this_works! + :ok +end +``` + +### Remove redundant final clause + +If the pattern of the final clause of the head is also the `with` statements `do` body, styler nixes the final match and makes the right hand side of the clause into the do body. + +```elixir +# Given +with {:ok, a} <- foo(), + {:ok, b} <- bar(a) do + {:ok, b} +else + error -> error +end +# Styled: +with {:ok, a} <- foo() do + bar(a) +end +``` + +### Replace with `case` + +A `with` statement with a single clause in the head and `else` is a really just a `case` clause putting on airs. + +```elixir +# Given: +with :ok <- foo do + :success +else + :fail -> :failure + error -> error +end +# Styled: +case foo do + :ok -> :success + :fail -> :failure + error -> error +end +``` + +### Replace with `if` + +Given Styler rewrites trivial `case` to `if`, it shouldn't be a surprise that that same rule means that `with` can be rewritten to `if` in some cases. + +```elixir +# Given: +with true <- foo(), bar <- baz() do + {:ok, bar} +else + _ -> :error +end +# Styled: +if foo() do + bar = baz() + {:ok, bar} +else + :error +end +``` diff --git a/docs/credo.cheatmd b/docs/credo.md similarity index 93% rename from docs/credo.cheatmd rename to docs/credo.md index 0b1597e..6a89b10 100644 --- a/docs/credo.cheatmd +++ b/docs/credo.md @@ -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.md b/docs/mix_configs.md new file mode 100644 index 0000000..c3b45e8 --- /dev/null +++ b/docs/mix_configs.md @@ -0,0 +1,18 @@ +# 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 has `import 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) + +## Examples + +TODOs diff --git a/docs/module_directives.md b/docs/module_directives.md new file mode 100644 index 0000000..0c725c4 --- /dev/null +++ b/docs/module_directives.md @@ -0,0 +1,177 @@ +## Adds Moduledoc + +Adds `@moduledoc false` to modules without a moduledoc unless the module's name ends with one of the following: + +* `Test` +* `Mixfile` +* `MixProject` +* `Controller` +* `Endpoint` +* `Repo` +* `Router` +* `Socket` +* `View` +* `HTML` +* `JSON` + +## Directive Expansion + +Expands `Module.{SubmoduleA, SubmoduleB}` to their explicit forms for ease of searching. + +```elixir +# Before +import Foo.{Bar, Baz, Bop} +alias Foo.{Bar, Baz.A, Bop} + +# After +import Foo.Bar +import Foo.Baz +import Foo.Bop + +alias Foo.Bar +alias Foo.Baz.A +alias Foo.Bop +``` + +## Directive Organization + +Modules directives are sorted into the following order: + +* `@shortdoc` +* `@moduledoc` (adds `@moduledoc false`) +* `@behaviour` +* `use` +* `import` (sorted alphabetically) +* `alias` (sorted alphabetically) +* `require` (sorted alphabetically) +* everything else (order unchanged) + +### Before + +```elixir +defmodule Foo do + @behaviour Lawful + alias A.A + require A + + use B + + def c(x), do: y + + import C + @behaviour Chaotic + @doc "d doc" + def d do + alias X.X + alias H.H + + alias Z.Z + import Ecto.Query + X.foo() + end + @shortdoc "it's pretty short" + import A + alias C.C + alias D.D + + require C + require B + + use A + + alias C.C + alias A.A + + @moduledoc "README.md" + |> File.read!() + |> String.split("") + |> Enum.fetch!(1) +end +``` + +### After + +```elixir +defmodule Foo do + @shortdoc "it's pretty short" + @moduledoc "README.md" + |> File.read!() + |> String.split("") + |> Enum.fetch!(1) + @behaviour Chaotic + @behaviour Lawful + + use B + use A.A + + import A.A + import C + + alias A.A + alias C.C + alias D.D + + require A + require B + require C + + def c(x), do: y + + @doc "d doc" + def d do + import Ecto.Query + + alias H.H + alias X.X + alias Z.Z + + X.foo() + end +end +``` + +If any line previously relied on an alias, the alias is fully expanded when it is moved above the alias: + +```elixir +# Given +alias Foo.Bar +import Bar +# Styled +import Foo.Bar + +alias Foo.Bar +``` + +## Alias Lifting + +When a module with three parts is referenced two or more times, styler creates a new alias for that module and uses it. + +```elixir +# Given +require A.B.C + +A.B.C.foo() +A.B.C.bar() + +# Styled +alias A.B.C + +require C + +C.foo() +C.bar() +``` + +### Collisions + +Styler won't lift aliases that will collide with existing aliases, and likewise won't lift any module whose name would collide with a standard library name. + +You can specify additional modules to exlude from lifting via the `:alias_lifting_exclude` configuration option. For the example above, the following configuration would keep Styler from creating the `alias A.B.C` node: + +```elixir +# .formatter.exs +[ + plugins: [Styler], + styler: [alias_lifting_exclude: [:C]], +] +``` diff --git a/docs/styles.cheatmd b/docs/styles.cheatmd deleted file mode 100644 index 4017997..0000000 --- a/docs/styles.cheatmd +++ /dev/null @@ -1,273 +0,0 @@ -# Styles - -## 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() -``` - -### Large Base 10 Numbers - -Style base 10 numbers with 5 or more digits to have a `_` every three digits. -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 -``` - -### `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 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 - -### 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. - -#### Before -```elixir -foo |> Keyword.merge(%{just_one_key: the_value}) |> bar() -``` - -#### After -```elixir -foo |> Keyword.put(:just_one_key, the_value) |> bar() -``` - -### 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) -``` - -### `Enum.reverse(foo) ++ bar -> Enum.reverse(foo, bar)` - -`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) -``` - -### 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() -``` - - -### DateModule.compare(x, y) == :lt/:gt -> 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! - -#### Before -```elixir -if DateTime.compare(start, end) == :gt, - do: :error, - else: :ok -``` -#### After -```elixir -if DateTime.after?(start, end), - do: :error, - else: :ok -``` - -### Code Readability - -- put matches on right -- `Credo.Check.Readability.PreferImplicitTry` - -### Consistency -- `def foo()` -> `def 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` - -1.16+ - -- 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`, ...) - -## 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) - -## Control Flow Structures (aka "Blocks": `case`, `if`, `unless`, `cond`, `with`) - -### `case` - -- rewrite to `if` for `true/false`, `true/_`, `false/true` - - -### `with` - -`with` great power comes a great responsibility. don't use `with` when another (simpler!) "Control Flow Structure" - -- single statement `with` with `else` clauses is rewritten to `case` (which can be further rewritten to an `if`!) -- move non `<-` out of the head and into preroll or body -- fully replace with statement with normal code as -- drop redundant identity else clause `else: (error -> error)` (also more complex matches, ala `{:error, error} -> {:error, error}`) -- Credo.Check.Refactor.RedundantWithClauseResult - -### `cond` -- Credo.Check.Refactor.CondStatements - -### `if`/`unless` - -if/unless often looks to see if the root of the statement is a "negator", defined as one of the following operators: `:!, :not, :!=, :!==`. We always try to rewrite if/unless statements to not be negated, using the inverse construct when appropriate (but we'll never write an unless with an `else`) - -- repeated negators (`!!`) are removed -- negated if/unless without an `else` are inverted to unless/if (this is done recursively until 0 or 1 negations remain) -- `unless` with `else` are inverted to negated `if` statements -- negated `if` with `else` have their clauses inverted to remove the negation -- if/unless with `else: nil` is dropped as redundant - -## Pipe Chains - -### Pipe Start - -- raw value -- blocks are extracted to variables -- ecto's `from` is allowed - -### Piped function rewrites - -- add parens to function calls `|> fun |>` => `|> fun() |>` -- remove unnecessary `then/2`: `|> then(&f(&1, ...))` -> `|> f(...)` -- add `then` when defining anon funs in pipe `|> (& &1).() |>` => `|> |> then(& &1) |>` - -### Piped function optimizations - -Two function calls into one! Tries to fit everything on one line when shrinking. - -- `lhs |> Enum.reverse() |> Enum.concat(enum)` => `lhs |> Enum.reverse(enum)` (also Kernel.++) -- `lhs |> Enum.filter(filterer) |> Enum.count()` => `lhs |> Enum.count(count)` -- `lhs |> Enum.map(mapper) |> Enum.join(joiner)` => `lhs |> Enum.map_join(joiner, mapper)` -- `lhs |> Enum.map(mapper) |> Enum.into(empty_map)` => `lhs |> Map.new(mapper)` -- `lhs |> Enum.map(mapper) |> Enum.into(collectable)` => `lhs |> Enum.into(collectable, mapper)` -- `lhs |> Enum.map(mapper) |> Map.new()` => `lhs |> Map.new(mapper)` mapset & keyword also - -### Unpiping Single Pipes - -- notably, optimizations might turn a 2 pipe into a single pipe -- doesn't unpipe when we're starting w/ quote -- pretty straight forward i daresay diff --git a/docs/styles.md b/docs/styles.md new file mode 100644 index 0000000..7b3d4f1 --- /dev/null +++ b/docs/styles.md @@ -0,0 +1,210 @@ +# 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. + +* `"{\"errors\":[\"Not Authorized\"]}"` => `~s({"errors":["Not Authorized"]})` + +## Large Base 10 Numbers + +Style base 10 numbers with 5 or more digits to have a `_` every three digits. +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 | After | +|--------|-------| +| `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` -> `X.new` + +This rewrite is applied when the collectable is a new map, keyword list, or mapset via `Enum.into/2,3`. + +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" + +| Before | After | +|--------|-------| +| `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. +| Before | After | +|--------|-------| +| `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 | After | +|--------|-------| +| `Map.drop(map, [key])` | `Map.delete(map, key)`| +| `Keyword.drop(kw, [key])` | `Keyword.delete(kw, key)`| + +## `Enum.reverse/1` and concatenation -> `Enum.reverse/2` + +`Enum.reverse/2` optimizes a two-step reverse and concatenation into a single step. + +| Before | After | +|--------|-------| +| `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 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. + +This also hews to our internal styleguide's "Don't make one-line helper functions" guidance. + +## `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! + +The examples below use `DateTime.compare/2`, but the same is also done for `NaiveDateTime|Time|Date.compare/2` + +| Before | After | +|--------|-------| +| `DateTime.compare(start, end_date) == :gt` | `DateTime.after?(start, end_date)` | +| `DateTime.compare(start, end_date) == :lt` | `DateTime.before?(start, end_date)` | + +## 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 +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 +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 +``` + +## Remove parenthesis from 0-arity function & macro definitions + +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. + +| Before | After | +|--------|-------| +| `def foo()` | `def foo`| +| `defp foo()` | `defp foo`| +| `defmacro foo()` | `defmacro foo`| +| `defmacrop foo()` | `defmacrop foo`| + +## Elixir Deprecation Rewrites + +### 1.15+ + +| Before | After | +|--------|-------| +| `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` *| +| `IO.read/bin_read` with `:all` option | replace `:all` with `:eof`| + +\* For both of the "decreasing range" changes, the rewrite can only be applied if the range is being passed as an argument to the function. + +### 1.16+ +| Before | After | +|--------|-------| +|`File.stream!(file, options, line_or_bytes)` | `File.stream!(file, line_or_bytes, options)`| + + +## Code Readability + +- put matches on right +- `Credo.Check.Readability.PreferImplicitTry` + +## Function Definitions + +- Shrink multi-line function defs +- Put assignments on the right + +## `cond` +- Credo.Check.Refactor.CondStatements + +# Pipe Chains + +## Pipe Start + +- raw value +- blocks are extracted to variables +- ecto's `from` is allowed + +## Piped function rewrites + +- add parens to function calls `|> fun |>` => `|> fun() |>` +- remove unnecessary `then/2`: `|> then(&f(&1, ...))` -> `|> f(...)` +- add `then` when defining anon funs in pipe `|> (& &1).() |>` => `|> |> then(& &1) |>` + +## Piped function optimizations + +Two function calls into one! Tries to fit everything on one line when shrinking. + +| Before | After | +|--------|-------| +| `lhs |> Enum.reverse() |> Enum.concat(enum)` | `lhs |> Enum.reverse(enum)` (also Kernel.++) | +| `lhs |> Enum.filter(filterer) |> Enum.count()` | `lhs |> Enum.count(count)` | +| `lhs |> Enum.map(mapper) |> Enum.join(joiner)` | `lhs |> Enum.map_join(joiner, mapper)` | +| `lhs |> Enum.map(mapper) |> Enum.into(empty_map)` | `lhs |> Map.new(mapper)` | +| `lhs |> Enum.map(mapper) |> Enum.into(collectable)` | `lhs |> Enum.into(collectable, mapper)` | +| `lhs |> Enum.map(mapper) |> Map.new()` | `lhs |> Map.new(mapper)` mapset & keyword also | + +## Unpiping Single Pipes + +- notably, optimizations might turn a 2 pipe into a single pipe +- doesn't unpipe when we're starting w/ quote +- pretty straight forward i daresay diff --git a/docs/troubleshooting.cheatmd b/docs/troubleshooting.cheatmd deleted file mode 100644 index 7daa9d3..0000000 --- a/docs/troubleshooting.cheatmd +++ /dev/null @@ -1,34 +0,0 @@ -### Troubleshooting: Compilation broke due to Module Directive rearrangement - -Styler naively moves module attributes, which can break compilation. For now, the only fix is some elbow grease. - -#### Module Attribute dependency - -Another common compilation break on the first run is a `@moduledoc` that depended on another module attribute which -was moved below it. - -For example, given the following broken code after an initial `mix format`: - -```elixir -defmodule MyGreatLibrary do - @moduledoc make_pretty_docs(@library_options) - use OptionsMagic, my_opts: @library_options - - @library_options [ ... ] -end -``` - -You can fix the code by moving the static value outside of the module into a naked variable and then reference it in the module. (Note that `use` macros need an `unquote` wrapping the variable!) - -Yes, this is a thing you can do in a `.ex` file =) - -```elixir -library_options = [ ... ] - -defmodule MyGreatLibrary do - @moduledoc make_pretty_docs(library_options) - use OptionsMagic, my_opts: unquote(library_options) - - @library_options library_options -end -``` diff --git a/lib/style/module_directives.ex b/lib/style/module_directives.ex index aeaeb8c..e385eb9 100644 --- a/lib/style/module_directives.ex +++ b/lib/style/module_directives.ex @@ -27,10 +27,6 @@ defmodule Styler.Style.ModuleDirectives do * `Credo.Check.Readability.UnnecessaryAliasExpansion` * `Credo.Check.Design.AliasUsage` - ## Breakages - - **This can break your code.** - ### Strict Layout Modules directives are sorted into the following order: @@ -43,48 +39,6 @@ defmodule Styler.Style.ModuleDirectives do * `alias` * `require` * everything else (unchanged) - - If any of the sorted directives had a dependency on code that is now below it, your code will fail to compile after being styled. - - For instance, the following will be broken because the module attribute definition will - be moved below the `use` clause, meaning `@pi` is undefined when invoked. - - ```elixir - # before - defmodule Approximation do - @pi 3.14 - use Math, pi: @pi - end - - # after - defmodule Approximation do - @moduledoc false - use Math, pi: @pi - @pi 3.14 - end - ``` - - For now, it's up to you to come up with a fix for this issue. Sorry! - - ### Strict Layout: interwoven conflicting aliases - - Ideally no one writes code like this as it's hard for our human brains to notice the context switching! - Still, it's a possible source of breakages in Styler. - - - alias Foo.Bar - Bar.Baz.bop() - - alias Baz.Bar - Bar.Baz.bop() - - # becomes - - alias Baz.Bar - alias Baz.Bar.Baz - alias Foo.Bar - Baz.bop() # was Foo.Bar.Baz, is now Baz.Bar.Baz - Baz.bop() """ @behaviour Styler.Style diff --git a/mix.exs b/mix.exs index a00cd5c..81fb9b9 100644 --- a/mix.exs +++ b/mix.exs @@ -58,10 +58,18 @@ defmodule Styler.MixProject do main: "readme", source_ref: "v#{@version}", source_url: @url, + groups_for_extras: [ + Rewrites: ~r/docs/ + ], + extra_section: "Docs", extras: [ "CHANGELOG.md": [title: "Changelog"], - "README.md": [title: "Styler"], - "docs/styles.cheatmd": [title: "Examples"] + "docs/styles.md": [title: "Simple Styles"], + "docs/control_flow_macros.md": [title: "Control Flow Macros (if, case, ...)"], + "docs/mix_configs.md": [title: "Mix Config Files (config/config.exs, ...)"], + "docs/module_directives.md": [title: "Module Directives (use, alias, ...)"], + "docs/credo.md": [title: "Styler & Credo"], + "README.md": [title: "Styler"] ] ] end