New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Format codebase files with the upcoming code formatter #6643

Closed
josevalim opened this Issue Oct 8, 2017 · 21 comments

Comments

Projects
None yet
8 participants
@josevalim
Member

josevalim commented Oct 8, 2017

Hi everyone!

Elixir master now includes a code formatter that automatically formats your code according to a consistent style.

We want to convert Elixir's codebase to use the formatter on all files. We understand this means a lot of code churn now but, on the positive side, it means we can automatically enforce and/or format future contributions, making it easier for everyone to contribute to Elixir.

We plan to migrate Elixir's source code to use the formatter gradually and we need your help! If you were looking forward to contribute to Elixir, this is a great opportunity to get started.

Please submit only one pull request per day with only one file formatted per PR, otherwise we are unable to give everyone feedback.

Helping with pull requests

You can contribute by picking a random file in Elixir's codebase, running the formatter on it, analysing it for improvements and then submitting a pull request.

The first step is to clone the Elixir repository and compile it:

$ git clone git@github.com:elixir-lang/elixir.git
$ cd elixir
$ make compile

If you have already cloned it, make sure you have the latest and run make clean compile again. You can read more about contributing in our README.

After you have latest Elixir on your machine, you need to pick a random file to format. We have added a script to do exactly that. From your Elixir checkout root:

$ bin/elixir scripts/random_file.exs

The script will tell you how to format a random file that has not been formatted yet. Run the command suggested by the script, check for style improvements and then submit a pull request for the changes on that single file. We will explain what are the possible improvements in the next section, so please look for them carefully. For more information on pull requests, read here.

Checking for style improvements

The formatter is guaranteed to spit out valid Elixir code. However, depending on how the original code was written, it may unecessarily span over multiple lines when formatted. In such cases, you may need to move some code around. Let's see some examples directly from the Elixir codebase.

Example 1: multi-line data structures

The example below uses Elixir old-style of indentation:

:ets.insert(table, {doc_tuple, line, kind,
                    merge_signatures(current_sign, signature, 1),
                    if(is_nil(doc), do: current_doc, else: doc)})

when formatted it looks like this:

:ets.insert(table, {
  doc_tuple,
  line,
  kind,
  merge_signatures(current_sign, signature, 1),
  if(is_nil(doc), do: current_doc, else: doc)
})

However, the code above only spawn multiple lines because we have a tuple full of expressions. If we move those expressions out, we will get better code altogether:

new_signature = merge_signatures(current_sign, signature, 1)
new_doc = if is_nil(doc), do: current_doc, else: doc
:ets.insert(table, {doc_tuple, line, kind, new_signature, new_doc})

After you do your changes, run the formatter again, see if the final code looks as you desire and ship it!

Example 2: multi-line function definitions

Sometimes a function definition may spawn over multiple lines:

def handle_call({:clean_up_process_data, parent_pid, child_pid}, _from,
                %{parent_pids: parent_pids, child_pids: child_pids} = state) do

When formatted, it will now look like:

def handle_call(
      {:clean_up_process_data, parent_pid, child_pid},
      _from,
      %{parent_pids: parent_pids, child_pids: child_pids} = state
    ) do

In some cases, the multi-line definition is necessary but, in this case, we could simply move the extraction of the state fields out of the function definition, since we are matching on any of them for flow-control:

def handle_call({:clean_up_process_data, parent_pid, child_pid}, _from, state) do
  %{parent_pids: parent_pids, child_pids: child_pids} = state

The code is now clearer and the function definition matches only on arguments that must be matched on the function head.

After you do your changes, run the formatter again, see if the final code looks as you desire and ship it!

Example 3: calls with many arguments

The example below is formatted by the formatter:

assert_raise EEx.SyntaxError,
             "nofile:2: unexpected end of string, expected a closing '<% end %>'",
             fn ->
               EEx.compile_string("foo\n<% if true do %>")
             end

This doesn't look ideal because of all the whitespace it leaves to the left of the code. In this case, prefer to extract arguments in variables so that they all fit on one line. This is especially easy in cases like the one above, because fn can be cleanly split at the end, so it's enough to extract the message into a variable (which is a common pattern in the Elixir codebase specifically regarding assert_raise/3):

message = "nofile:2: unexpected end of string, expected a closing '<% end %>'"

assert_raise EEx.SyntaxError, message, fn ->
  EEx.compile_string("foo\n<% if true do %>")
end

After you do your changes, run the formatter again, see if the final code looks as you desire and ship it!

Example 4: line splits in ugly places

The example below is formatted by the formatter:

"Finished in #{format_us(total_us)} seconds (#{format_us(load_us)}s on load, #{
  format_us(run_us)
}s on tests)"

As you can see, the line split happens on #{. This is not ideal, so in cases similar to this, the correct thing to do is to split the string into multiple strings and concatenate them through <>:

"Finished in #{format_us(total_us)} seconds (#{format_us(load_us)}s on load, " <>
  "#{format_us(run_us)}s on tests)"

After you do your changes, run the formatter again, see if the final code looks as you desire and ship it!

Summing up

Let us know if you have any questions and we are looking forward to your contributions!

@whatyouhide

This comment has been minimized.

Show comment
Hide comment
@whatyouhide

whatyouhide Oct 9, 2017

Member

I performed the steps José suggested and came up with the first PR, #6652. If you're in doubt you can use it as an example :).

Member

whatyouhide commented Oct 9, 2017

I performed the steps José suggested and came up with the first PR, #6652. If you're in doubt you can use it as an example :).

@LostKobrakai

This comment has been minimized.

Show comment
Hide comment
@LostKobrakai

LostKobrakai Oct 9, 2017

Contributor

How about nested function calls to pipes? I've got elixir/lib/port.ex and there are a few line like that after formatting: nillify(:erlang.port_info(port, item)). It could be written as port |> :erlang.port_info(item) |> nillify()

Contributor

LostKobrakai commented Oct 9, 2017

How about nested function calls to pipes? I've got elixir/lib/port.ex and there are a few line like that after formatting: nillify(:erlang.port_info(port, item)). It could be written as port |> :erlang.port_info(item) |> nillify()

@josevalim

This comment has been minimized.

Show comment
Hide comment
@josevalim

josevalim Oct 9, 2017

Member

@LostKobrakai leave those as is especially because many modules in Elixir core cannot use pipes because of bootstrapping issues.

Member

josevalim commented Oct 9, 2017

@LostKobrakai leave those as is especially because many modules in Elixir core cannot use pipes because of bootstrapping issues.

@gmile

This comment has been minimized.

Show comment
Hide comment
@gmile

gmile Oct 9, 2017

Contributor

@josevalim running locally, I'm seeing the following code change introduced by formatter:

-    assert_raise RuntimeError, "the given function must return a two-element tuple or :pop, got: 1", fn ->
-      Keyword.get_and_update!([a: 1], :a, fn value -> value end)
-    end
+    assert_raise RuntimeError,
+                 "the given function must return a two-element tuple or :pop, got: 1",
+                 fn ->
+                   Keyword.get_and_update!([a: 1], :a, fn value -> value end)
+                 end

What would be a preferred way to fix this one? If I am to asked, I'd keep the original version as it looks fine?

Contributor

gmile commented Oct 9, 2017

@josevalim running locally, I'm seeing the following code change introduced by formatter:

-    assert_raise RuntimeError, "the given function must return a two-element tuple or :pop, got: 1", fn ->
-      Keyword.get_and_update!([a: 1], :a, fn value -> value end)
-    end
+    assert_raise RuntimeError,
+                 "the given function must return a two-element tuple or :pop, got: 1",
+                 fn ->
+                   Keyword.get_and_update!([a: 1], :a, fn value -> value end)
+                 end

What would be a preferred way to fix this one? If I am to asked, I'd keep the original version as it looks fine?

@josevalim

This comment has been minimized.

Show comment
Hide comment
@josevalim

josevalim Oct 9, 2017

Member

@gmile move the message out!

msg = "... long string ..."
assert_raise RuntimeError, msg, fn ->
  ...
end
Member

josevalim commented Oct 9, 2017

@gmile move the message out!

msg = "... long string ..."
assert_raise RuntimeError, msg, fn ->
  ...
end
@gmile

This comment has been minimized.

Show comment
Hide comment
@gmile

gmile Oct 9, 2017

Contributor

@josevalim thanks! done in #6690

Contributor

gmile commented Oct 9, 2017

@josevalim thanks! done in #6690

@josevalim

This comment has been minimized.

Show comment
Hide comment
@josevalim

josevalim Oct 9, 2017

Member

Folks running the formatter multiple times, please don't forget to frequently pull from master. :) Thank you!

Member

josevalim commented Oct 9, 2017

Folks running the formatter multiple times, please don't forget to frequently pull from master. :) Thank you!

@LostKobrakai

This comment has been minimized.

Show comment
Hide comment
@LostKobrakai

LostKobrakai Oct 9, 2017

Contributor

And do not forget to recompile after pulling :D

Contributor

LostKobrakai commented Oct 9, 2017

And do not forget to recompile after pulling :D

@josevalim

This comment has been minimized.

Show comment
Hide comment
@josevalim

josevalim Oct 9, 2017

Member

Ok folks, this was a bigger success than I expected. :D If you have already sent a PRs today, please hold back until we clear up the backlog. I am afraid we may find bugs in the formatter that will generate rework on later steps. Thanks everyone so far! I will let you know as soon as we are ready to handle more! ❤️

Member

josevalim commented Oct 9, 2017

Ok folks, this was a bigger success than I expected. :D If you have already sent a PRs today, please hold back until we clear up the backlog. I am afraid we may find bugs in the formatter that will generate rework on later steps. Thanks everyone so far! I will let you know as soon as we are ready to handle more! ❤️

@LostKobrakai

This comment has been minimized.

Show comment
Hide comment
@LostKobrakai

LostKobrakai Oct 9, 2017

Contributor

Also travis seems to run rather hot today :P

Contributor

LostKobrakai commented Oct 9, 2017

Also travis seems to run rather hot today :P

@josevalim

This comment has been minimized.

Show comment
Hide comment
@josevalim

josevalim Oct 10, 2017

Member

Thanks for everyone who contributed in the last 24 hours! After your contributions, 37% of the codebase is now properly formatted (it was around 8% when we started).

$ elixir scripts/random_file.exs --stat
169 out of 452 files formatted

We welcome more contributions but note we are keeping the restriction of one pull request per day so we can give feedback to everyone. Also please check if your previous PR have been merged or if there is any work pending. ❤️

Member

josevalim commented Oct 10, 2017

Thanks for everyone who contributed in the last 24 hours! After your contributions, 37% of the codebase is now properly formatted (it was around 8% when we started).

$ elixir scripts/random_file.exs --stat
169 out of 452 files formatted

We welcome more contributions but note we are keeping the restriction of one pull request per day so we can give feedback to everyone. Also please check if your previous PR have been merged or if there is any work pending. ❤️

@glazzari

This comment has been minimized.

Show comment
Hide comment
@glazzari

glazzari Oct 11, 2017

Contributor

How about raise/2? Should we extract the message out to a variable?

-        raise ArgumentError, "setup/setup_all expect a callback name as an atom or " <>
-                             "a list of callback names, got: #{inspect k}"
+        raise ArgumentError,
+              "setup/setup_all expect a callback name as an atom or " <>
+                "a list of callback names, got: #{inspect(k)}"
Contributor

glazzari commented Oct 11, 2017

How about raise/2? Should we extract the message out to a variable?

-        raise ArgumentError, "setup/setup_all expect a callback name as an atom or " <>
-                             "a list of callback names, got: #{inspect k}"
+        raise ArgumentError,
+              "setup/setup_all expect a callback name as an atom or " <>
+                "a list of callback names, got: #{inspect(k)}"
@josevalim

This comment has been minimized.

Show comment
Hide comment
@josevalim

josevalim Oct 11, 2017

Member

@guilhermelazzari only if extracting to a variable makes both the variable definition and raise fit on the same line. The general principle is that we want to avoid multi-lines, unless there is no way around it (which is the case above).

Member

josevalim commented Oct 11, 2017

@guilhermelazzari only if extracting to a variable makes both the variable definition and raise fit on the same line. The general principle is that we want to avoid multi-lines, unless there is no way around it (which is the case above).

@iJackUA

This comment has been minimized.

Show comment
Hide comment
@iJackUA

iJackUA Oct 11, 2017

Contributor

@josevalim do you have kind of official code style guide in written form like
https://github.com/christopheradams/elixir_style_guide or https://github.com/rrrene/elixir-style-guide
where current Formatter rules are specified?
(But I suppose no, as rules are still being updated).

Contributor

iJackUA commented Oct 11, 2017

@josevalim do you have kind of official code style guide in written form like
https://github.com/christopheradams/elixir_style_guide or https://github.com/rrrene/elixir-style-guide
where current Formatter rules are specified?
(But I suppose no, as rules are still being updated).

@whatyouhide

This comment has been minimized.

Show comment
Hide comment
@whatyouhide

whatyouhide Oct 11, 2017

Member

@iJackUA the rules are still being formed but the closest style guide is https://github.com/lexmag/elixir-style-guide which is maintained by core team members :).

Member

whatyouhide commented Oct 11, 2017

@iJackUA the rules are still being formed but the closest style guide is https://github.com/lexmag/elixir-style-guide which is maintained by core team members :).

@josevalim

This comment has been minimized.

Show comment
Hide comment
@josevalim

josevalim Oct 11, 2017

Member

Alright folks, this is it! There are only 15 files left so we are closing this! If you have already started working on something, then please still submit your PR, but don't start working on anything new since with 15 files left the chance of getting a conflict is quite high.

And those that have PRs open, please finalize it. :)

Tomorrow I will send some statistics about the work done. :) Thanks everyone! ❤️ 💚 💙 💛 💜

Member

josevalim commented Oct 11, 2017

Alright folks, this is it! There are only 15 files left so we are closing this! If you have already started working on something, then please still submit your PR, but don't start working on anything new since with 15 files left the chance of getting a conflict is quite high.

And those that have PRs open, please finalize it. :)

Tomorrow I will send some statistics about the work done. :) Thanks everyone! ❤️ 💚 💙 💛 💜

@zeroum

This comment has been minimized.

Show comment
Hide comment
@zeroum

zeroum Oct 11, 2017

Contributor

Ohhh, 2 minutes late!

Contributor

zeroum commented Oct 11, 2017

Ohhh, 2 minutes late!

@josevalim

This comment has been minimized.

Show comment
Hide comment
@josevalim

josevalim Oct 12, 2017

Member

Thanks everyone who contributed! All files in the Elixir codebase have been formatted and now all future contributions expect the files to be formatted, otherwise CI will fail.

Overall we formatted 452 files. We merged 214 Pull requests merged by 84 people with a total of 368 commits pushed to master.

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

❤️ 💚 💙 💛 💜

Member

josevalim commented Oct 12, 2017

Thanks everyone who contributed! All files in the Elixir codebase have been formatted and now all future contributions expect the files to be formatted, otherwise CI will fail.

Overall we formatted 452 files. We merged 214 Pull requests merged by 84 people with a total of 368 commits pushed to master.

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

❤️ 💚 💙 💛 💜

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment