Elixir 1.6 code formatter

[sasa1977]

@sebastian @obrok @cristianberneanu

Since at this point we use Elixir 1.6, we should discuss when/how shall we start using the formatter.

I did a quick experiment, and my impression is that formatting will touch many lines in pretty much all of our source files. Therefore, I think that once we decide to use the formatter, we should format all the files at once, and make CI fail if the code is not properly formatted.

The question is when do we want to do this. We could start early, and be done with it, or alternatively wait for the next version and see if the formatter becomes a bit less opinionated than it is today.

Thoughts?

[sebastian]

Well, if it becomes less opinionated later, then it we can let it format the code in a nicer way again. I would say let's do it sooner rather than later. No need to wait.

However, do you have a sample change you could post? Just to get a sense of what the style looks like (and how much I need to cry)?

[cristianberneanu]

Since it would be pretty disruptive, I would vote that we wait.

[sebastian]

Since it would be pretty disruptive, I would vote that we wait.

Wait for what? Or rather, since this is a disruptive change, will there ever actually be a time that is better?

[cristianberneanu]

Wait for the next version in the hope that it is more permissive. Changing all the files will affect the commit history (it will break blame) and also result in merge conflicts between the master and other branches.

[sasa1977]

I included an example diff for one file at the end of this comment. I think we can increase the default line length (default is 98), thought the question is do we want to tweak anything, or just stick with community defaults.

Among other things, the formatter doesn't permit two consecutive empty lines, and it inserts one empty line between clauses of the same function.

Wait for what?

Currently the formatter is very opinionated and has almost no configuration. I don't think this will change significantly, and perhaps we should just go with it immediately. However, if the formatter becomes more relaxed later, we won't profit from that if we convert our code now. For example, the next version of the formatter might be more respective of the vertical spacing and allow two consecutive empty lines. However, if we format our code now, we'll lose our current vertical style forever.

On the other hand, we don't know whether the formatter will be more permissive in the future, and waiting for the next version means six more months of manually dealing with the layout, so that's a strong argument for making the change sooner.

Also, you should check whether your editor has the support for auto formatting files on save. However, you should only turn this on once we format our code, and the changes are merged.

Here's an example diff:

diff --git a/cloak/lib/cloak/memory_reader.ex b/cloak/lib/cloak/memory_reader.ex
index 2050645..7481210 100644
--- a/cloak/lib/cloak/memory_reader.ex
+++ b/cloak/lib/cloak/memory_reader.ex
@@ -17,26 +17,25 @@ defmodule Cloak.MemoryReader do

   @type query_killer_callbacks :: {(() -> :ok), (() -> :ok)}

-
   # -------------------------------------------------------------------
   # API functions
   # -------------------------------------------------------------------

   @doc "Starts the memory reader server"
-  @spec start_link() :: GenServer.on_start
+  @spec start_link() :: GenServer.on_start()
   def start_link(), do: GenServer.start_link(__MODULE__, [], name: __MODULE__)

   @doc "Registers a query such that it can later be killed in case of a low memory event"
   @spec query_registering_callbacks() :: query_killer_callbacks
   def query_registering_callbacks() do
     query_pid = self()
+
     {
-      fn() -> GenServer.cast(__MODULE__, {:register_query, query_pid}) end,
-      fn() -> GenServer.cast(__MODULE__, {:unregister_query, query_pid}) end
+      fn -> GenServer.cast(__MODULE__, {:register_query, query_pid}) end,
+      fn -> GenServer.cast(__MODULE__, {:unregister_query, query_pid}) end
     }
   end

-
   # -------------------------------------------------------------------
   # GenServer callbacks
   # -------------------------------------------------------------------
@@ -48,75 +47,105 @@ defmodule Cloak.MemoryReader do
     # queries that run rampant to retain a stable `cloak`.
     :erlang.process_flag(:priority, :high)
     params = read_params()
+
     state = %{
       memory_projector: MemoryProjector.new(),
       queries: [],
       params: params,
       last_reading: nil,
-      readings: Readings.new([
-        {"current", 1},
-        {"last_5_seconds", 5 * measurements_per_second(%{params: params})},
-        {"last_1_minute", 12},
-        {"last_5_minutes", 5},
-        {"last_15_minutes", 3},
-        {"last_1_hour", 4},
-      ]),
+      readings:
+        Readings.new([
+          {"current", 1},
+          {"last_5_seconds", 5 * measurements_per_second(%{params: params})},
+          {"last_1_minute", 12},
+          {"last_5_minutes", 5},
+          {"last_15_minutes", 3},
+          {"last_1_hour", 4}
+        ])
     }
+
     :timer.send_interval(:timer.seconds(5), :report_memory_stats)
     schedule_check(state)
     {:ok, state}
   end

   @impl GenServer
-  def handle_cast({:unregister_query, pid}, %{queries: queries} = state), do:
-    {:noreply, %{state | queries: Enum.reject(queries, & &1 == pid)}}
+  def handle_cast({:unregister_query, pid}, %{queries: queries} = state),
+    do: {:noreply, %{state | queries: Enum.reject(queries, &(&1 == pid))}}
+
   def handle_cast({:register_query, pid}, %{queries: queries} = state) do
     Process.monitor(pid)
     {:noreply, %{state | queries: [pid | queries]}}
   end

   @impl GenServer
-  def handle_info({:DOWN, _monitor_ref, :process, pid, _info}, %{queries: queries} = state), do:
-    {:noreply, %{state | queries: Enum.reject(queries, & &1 == pid)}}
+  def handle_info({:DOWN, _monitor_ref, :process, pid, _info}, %{queries: queries} = state),
+    do: {:noreply, %{state | queries: Enum.reject(queries, &(&1 == pid))}}
+
   def handle_info(:read_memory, %{memory_projector: projector} = state) do
     reading = ProcMemInfo.read()
     time = System.monotonic_time(:millisecond)
     schedule_check(state)
+
     state
     |> record_reading(reading)
-    |> Map.put(:memory_projector, MemoryProjector.add_reading(projector, reading.available_memory, time))
+    |> Map.put(
+      :memory_projector,
+      MemoryProjector.add_reading(projector, reading.available_memory, time)
+    )
     |> perform_memory_check(reading.available_memory)
   end
-  def handle_info(:report_memory_stats, %{last_reading: nil} = state), do:
-    {:noreply, state}
+
+  def handle_info(:report_memory_stats, %{last_reading: nil} = state), do: {:noreply, state}
+
   def handle_info(:report_memory_stats, state) do
     payload = %{
       total_memory: state.last_reading.total_memory,
-      available_memory: Readings.values(state.readings),
+      available_memory: Readings.values(state.readings)
     }
+
     Cloak.AirSocket.send_memory_stats(payload)
     {:noreply, state}
   end

-
   # -------------------------------------------------------------------
   # Internal functions
   # -------------------------------------------------------------------

-  defp perform_memory_check(%{params: %{limit_to_start_checks: limit_to_start_checks}} = state, available_memory)
-      when available_memory > limit_to_start_checks, do:
-    {:noreply, state}
-  defp perform_memory_check(%{params: %{limit_to_check_for: memory_limit}} = state, available_memory)
-      when available_memory < memory_limit, do:
-    kill_query(state)
-  defp perform_memory_check(%{params: %{limit_to_check_for: memory_limit, allowed_minimum_time_to_limit: time_limit},
-      memory_projector: projector} = state, available_memory) do
+  defp perform_memory_check(
+         %{params: %{limit_to_start_checks: limit_to_start_checks}} = state,
+         available_memory
+       )
+       when available_memory > limit_to_start_checks,
+       do: {:noreply, state}
+
+  defp perform_memory_check(
+         %{params: %{limit_to_check_for: memory_limit}} = state,
+         available_memory
+       )
+       when available_memory < memory_limit,
+       do: kill_query(state)
+
+  defp perform_memory_check(
+         %{
+           params: %{limit_to_check_for: memory_limit, allowed_minimum_time_to_limit: time_limit},
+           memory_projector: projector
+         } = state,
+         available_memory
+       ) do
     case MemoryProjector.time_until_limit(projector, memory_limit) do
       {:ok, time} when time <= time_limit ->
-        Logger.error("Dangerous memory situation. Anticipating reaching the low memory threshold " <>
-          "(#{to_mb(memory_limit)} MB) in #{to_sec(time)} seconds. Available memory: #{to_mb(available_memory)} MB")
+        Logger.error(
+          "Dangerous memory situation. Anticipating reaching the low memory threshold " <>
+            "(#{to_mb(memory_limit)} MB) in #{to_sec(time)} seconds. Available memory: #{
+              to_mb(available_memory)
+            } MB"
+        )
+
         kill_query(state)
-      _ -> {:noreply, state}
+
+      _ ->
+        {:noreply, state}
     end
   end

@@ -124,55 +153,74 @@ defmodule Cloak.MemoryReader do

   defp to_sec(ms), do: max(trunc(ms / 1_000), 0)

-  defp kill_query(%{queries: []} = state), do:
-    {:noreply, state}
+  defp kill_query(%{queries: []} = state), do: {:noreply, state}
+
   defp kill_query(%{queries: [query | queries]} = state) do
     Cloak.Query.Runner.stop(query, :oom)
-    state = %{state |
-      # This adds an artificial cool down period between consecutive killings, proportional
-      # to the length of the memory projection buffer.
-      memory_projector: MemoryProjector.drop(state.memory_projector, num_measurements_to_drop(state)),
-      queries: queries,
+
+    state = %{
+      state
+      | # This adds an artificial cool down period between consecutive killings, proportional
+        # to the length of the memory projection buffer.
+        memory_projector:
+          MemoryProjector.drop(state.memory_projector, num_measurements_to_drop(state)),
+        queries: queries
     }
+
     {:noreply, state}
   end

-  defp schedule_check(%{params: %{check_interval: interval}}), do: Process.send_after(self(), :read_memory, interval)
+  defp schedule_check(%{params: %{check_interval: interval}}),
+    do: Process.send_after(self(), :read_memory, interval)

   def read_params() do
-    defaults = Application.fetch_env!(:cloak, :memory_limits)
-    |> Enum.reduce(%{}, fn({key, value}, map) -> Map.put(map, key, value) end)
-
-    config = case Aircloak.DeployConfig.fetch(:cloak, "memory_limits") do
-      {:ok, analyst_config} ->
-        [
-          :check_interval, :limit_to_start_checks, :limit_to_check_for,
-          :allowed_minimum_time_to_limit, :time_between_abortions
-        ]
-        |> Enum.reduce(%{}, fn(parameter, config) ->
-          value = Map.get(analyst_config, Atom.to_string(parameter), defaults[parameter])
-          Map.put(config, parameter, value)
-        end)
-      :error -> defaults
-    end
+    defaults =
+      Application.fetch_env!(:cloak, :memory_limits)
+      |> Enum.reduce(%{}, fn {key, value}, map -> Map.put(map, key, value) end)
+
+    config =
+      case Aircloak.DeployConfig.fetch(:cloak, "memory_limits") do
+        {:ok, analyst_config} ->
+          [
+            :check_interval,
+            :limit_to_start_checks,
+            :limit_to_check_for,
+            :allowed_minimum_time_to_limit,
+            :time_between_abortions
+          ]
+          |> Enum.reduce(%{}, fn parameter, config ->
+            value = Map.get(analyst_config, Atom.to_string(parameter), defaults[parameter])
+            Map.put(config, parameter, value)
+          end)
+
+        :error ->
+          defaults
+      end
+
+    Logger.debug(
+      "The low memory monitor is configured with: check_interval: #{config.check_interval} ms, " <>
+        "limit_to_start_checks: #{config.limit_to_start_checks} bytes, limit_to_check_for: " <>
+        "#{config.limit_to_check_for} bytes, allowed_minimum_time_to_limit: " <>
+        "#{config.allowed_minimum_time_to_limit} ms, and up to #{config.time_between_abortions} ms " <>
+        "of time waited between consecutive query abortions."
+    )

-    Logger.debug("The low memory monitor is configured with: check_interval: #{config.check_interval} ms, " <>
-      "limit_to_start_checks: #{config.limit_to_start_checks} bytes, limit_to_check_for: " <>
-      "#{config.limit_to_check_for} bytes, allowed_minimum_time_to_limit: " <>
-      "#{config.allowed_minimum_time_to_limit} ms, and up to #{config.time_between_abortions} ms " <>
-      "of time waited between consecutive query abortions.")
     config
   end

   defp record_reading(state, reading) do
-    %{state |
-      last_reading: reading,
-      readings: Readings.add_reading(state.readings, reading.available_memory),
+    %{
+      state
+      | last_reading: reading,
+        readings: Readings.add_reading(state.readings, reading.available_memory)
     }
   end

-  defp measurements_per_second(%{params: %{check_interval: interval}}), do: max(div(1_000, interval), 1)
+  defp measurements_per_second(%{params: %{check_interval: interval}}),
+    do: max(div(1_000, interval), 1)

-  defp num_measurements_to_drop(%{params: %{check_interval: interval, time_between_abortions: pause}}), do:
-    div(pause, interval)
+  defp num_measurements_to_drop(%{
+         params: %{check_interval: interval, time_between_abortions: pause}
+       }),
+       do: div(pause, interval)
 end

[sebastian]

Oh my! I hate these changes... Let's wait and see if it becomes less opinionated!

[sasa1977]

Changing all the files will affect the commit history (it will break blame)

I don't think we can do much here. Sooner or later we'll go for the formatter, and we'll end up with this state.

and also result in merge conflicts between the master and other branches.

Also something we'll need to deal with sooner or later, but I don't think it's going to be a huge problem.

You could resolve the conflict by:

• Taking all the changes from the unformatted version
• Reformatting the file

[sebastian]

I suggest we move this decision to the point where we are about to make a new release? If we do it right before we branch off the new release branch then we'll have consistent style across the release and the master branch, reducing the differences between the branches we have to maintain.

[sasa1977]

Oh my! I hate these changes... Let's wait and see if it becomes less opinionated!

Fair enough. However, I think we should use the formatter in the next version, regardless of how it works out. It doesn't make sense for us to diverge from the prevalent community style.

I suggest we move this decision to the point where we are about to make a new release? If we do it right before we branch off the new release branch then we'll have consistent style across the release and the master branch, reducing the differences between the branches we have to maintain.

Good idea!

[sasa1977]

Oh my! I hate these changes... Let's wait and see if it becomes less opinionated!

Which changes do you hate in particular?

[obrok]

1. I doubt it will become less opinionated. It might become opinionated in a different way, but I wouldn't count that would go our way either.
2. blame is nice, but there is always git log -L allowing you to track the history of a given line
3. IMO we should do it ASAP, because we already (kinda) agreed we want to, and it's not going to get better.
4. We could maybe do it piecemeal instead of in a huge PR. We could maintain a list of files that will be checked on CI and submit small PRs with one file, like the Elixir team did. Or just format the whole project and deal with refactorings later, when one has to view the code.

[sebastian]

Which changes do you hate in particular?

It uses vertical space where I don't want any, and none where I like it 😏 My main gripe is that it seems it's now less visually clear where one function ends and the next one starts. Every function instance is now broken apart with a single empty line, whether or not it's just a case in a function or a different function all together. And likewise it seems to add unnecessary empty lines within functions too.

IMO we should do it ASAP, because we already (kinda) agreed we want to, and it's not going to get better.

We are going to use the formatter, that part is pretty much settled. Avoiding spending time on manually formatting code is good. But what speaks against waiting for the next release branch such that we have uniform style across the release-branch and the master-branch? Makes it easier to go back and forth? Although since the step of getting the code formatted is a trivial and automated one, we could perform it on our current release-branch too. That would allow us doing it at once.

@sasa1977 there are no formatting options?

[sebastian]

Oh, it seems line length can be configured already with the :line_length option.

[sebastian]

From the docs:

The second principle is to provide as little configuration as possible. This eases the formatter adoption by removing contention points while making sure a single style is followed consistently by the community as a whole.

If this is the attitude, then it seems unlikely that much is going to change for a next version.

[sebastian]

Oh, formatting in the current release-branch obviously isn't possible, since we aren't on Elixir 1.6 there.

[sebastian]

At least let's agree on the following: for our new rust code, let's definitively use their formatter from day 1.

[sebastian]

Ok, looking at the elixir code base discussion where they reformatted the whole elixir codebase:

This was fun but hopefully we don't have to do something like this again soon! 😁

Hence:

• it sounds unlikely they are going to introduce major changes in the defaults
• configurability seems to be something they explicitly wanted to avoid

Therefore:

• let's perform the auto-styling at the point we are creating a new release branch (i.e. this will be in a couple of months)
• let's configure it to keep using our 120 character line lengths
• let's not waste time on correcting stylistic changes in pull-requests until then, but still follow our guidelines to the best of our built in abilities
• once we have made the move we should make it part of CI, rejecting non-confirming builds

For vim-users, check the bottom of this article for how to get formatting on save in vim.

[sasa1977]

Oh, it seems line length can be configured already with the :line_length option.

Yes, I mentioned that somewhere in this discussion. The question, is do we want to use our own line length, or just stick with the official defaults.

As you've seen yourself, the formatter is quite opinionated and doesn't leave a lot of room for configurations.

If this is the attitude, then it seems unlikely that much is going to change for a next version.

One thing I hope is that they will allow (or maybe even enforce) trailing commas in lists. I've seen some complaints about that. I also hope they maybe come around and decide to respect users vertical spacing, or at least provide some solution out of the box (perhaps in the shape of a generated comment before a multiclause). But all of this is just guesswork.

My main gripe is that it seems it's now less visually clear where one function ends and the next one starts. Every function instance is now broken apart with a single empty line, whether or not it's just a case in a function or a different function all together.

This is my biggest complaint too! For callback functions, this is solved with @impl which serves as a visual separator. The same holds for all public multiclauses (owing to docs/specs). But a private multiclause is not separated at all from other private functions, and the only solution I see here is to use comments.

[cristianberneanu]

I wonder how this discussion would have went if they would have settled on tabs instead of spaces :)

[sebastian]

I wonder how this discussion would have went if they would have settled on tabs instead of spaces :)

We clearly wouldn't be having this discussion at all then

---

When we move over to auto-formatting, let's do it wholesale and use something similar for javascript, HTML, and CSS too. Two options for that seem to be prettier and js-beautify.

I have also updated our style guide to include a top-level section that auto-formatting is used wherever it is available.

[obrok]

I on the other hand think the clauses being separated by an empty line is a nice improvement over our style. It seems much clearer to me. The compiler enforces or maybe warns when you mix clauses from different functions anyway, doesn't it?

[sebastian]

We are now using the formatter (with mixed levels of appreciation).