Skip to content

Format the codebase #657

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

Merged
merged 3 commits into from
Feb 13, 2018
Merged

Format the codebase #657

merged 3 commits into from
Feb 13, 2018

Conversation

whatyouhide
Copy link
Member

It's a big diff but not a lot to check out :)

One thing to check is .travis.yml, which I updated to check for formatted files on CI.

@@ -13,14 +13,22 @@ defmodule Plug.Adapters.Translator do
"""

# cowboy 1 format
def translate(min_level, :error, :format,
{~c"Ranch listener" ++ _, [ref, protocol, pid, reason]}) do
def translate(
Copy link
Contributor

@fertapric fertapric Feb 13, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are some of the changes on this file examples of "Example 2: multi-line function definitions"? Link: elixir-lang/elixir#6643

def translate(min_level, :error, :format, message) do
  {~c"Ranch listener" ++ _, [ref, protocol, pid, reason]} = message
  # ...
end

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We need to match on the top, unfortunately.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What about?

def translate(min_level, :error, :format, data) do
  case data do
    # cowboy 1 format
    {~c"Ranch listener" ++ _, [ref, protocol, pid, reason]} ->
      translate_ranch(min_level, ref, protocol, pid, reason)

    # cowboy 2 format
    {~c"Ranch listener" ++ _, [ref, pid, _stream_id, _stream_pid, reason, _]} ->
      translate_ranch(min_level, ref, :cowboy_protocol, pid, reason)
      
    _ ->
      :none
  end
end

def translate(_min_level, _level, _kind, _data), do: :none

_ = Plug.Conn.Status.code(status)
raise AlreadySentError
end

def send_file(%Conn{adapter: {adapter, payload}, owner: owner} = conn, status, file, offset, length) when is_binary(file) do
def send_file(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here:

def send_file(conn, status, file, offset, length) when is_binary(file) do
  %Conn{adapter: {adapter, payload}, owner: owner} = conn
  # ...
end

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would just match on the %Conn{} in the head because of better error messages in Elixir v1.5.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I completely forgot that when copy/pasting the param 😅 I've also updated the rest of the comments to match on %Conn{} = conn

@@ -683,8 +703,8 @@ defmodule Plug.Conn do
raise AlreadySentError
end

def delete_resp_header(%Conn{resp_headers: headers} = conn, key) when
is_binary(key) do
def delete_resp_header(%Conn{resp_headers: headers} = conn, key)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe?

def delete_resp_header(%Conn{} = conn, key) when is_binary(key) do
  %{conn | resp_headers: List.keydelete(conn.resp_headers, key, 0)}
end

@@ -542,8 +561,8 @@ defmodule Plug.Conn do
raise AlreadySentError
end

def put_req_header(%Conn{adapter: adapter, req_headers: headers} = conn, key, value) when
is_binary(key) and is_binary(value) do
def put_req_header(%Conn{adapter: adapter, req_headers: headers} = conn, key, value)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe?

def put_req_header(%Conn{} = conn, key, value) when is_binary(key) and is_binary(value) do
  validate_header_key_if_test!(conn.adapter, key)
  %{conn | req_headers: List.keystore(conn.req_headers, key, 0, {key, value})}
end

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Doesn't change much, I prefer the way it is right now.

def fetch_query_params(conn, opts \\ [])

def fetch_query_params(%Conn{query_params: %Unfetched{}, params: params,
query_string: query_string} = conn, opts) do
def fetch_query_params(
Copy link
Contributor

@fertapric fertapric Feb 13, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe?

def fetch_query_params(%Conn{} = conn, opts) do
  %Conn{query_params: %Unfetched{}, params: params, query_string: query_string} = conn
  # ...
end

def fetch_cookies(%Conn{req_cookies: %Unfetched{},
resp_cookies: resp_cookies,
req_headers: req_headers} = conn, _opts) do
def fetch_cookies(
Copy link
Contributor

@fertapric fertapric Feb 13, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe?

def fetch_cookies(%Conn{} = conn, _opts) do
  %Conn{req_cookies: %Unfetched{}, resp_cookies: resp_cookies, req_headers: req_headers} = conn
  # ...
end

" " <> padded_hour <> ":" <> padded_minute <>
":" <> padded_second <> " GMT"
binary_year = Integer.to_string(year)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe?

time = padded_hour <> ":" <> padded_minute <> ":" <> padded_second <> " GMT"
date = padded_day <> " " <> month_name <> " " <> binary_year
weekday_name <> ", " <> date <> time

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should use iodata to binary.

with [plain_text, signature] when byte_size(plain_text) > 0 and byte_size(signature) > 0 <- split,
{:ok, payload} <- decode_legacy_base64(plain_text),
{:ok, signature} <- decode_legacy_base64(signature) do
with [plain_text, signature] when byte_size(plain_text) > 0 and byte_size(signature) > 0 <-
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not 100% sure about this one, but the split in the new line looks a bit weird. Here is a suggestion:

with [plain_text, signature] = split,
     true <- byte_size(plain_text) > 0,
     true <- byte_size(signature) > 0,
     {:ok, payload} <- decode_legacy_base64(plain_text),
     {:ok, signature} <- decode_legacy_base64(signature) do
  {"HS1", payload, plain_text, signature}
else
  _ -> :error
end

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we can do plain_text != "" :)

" " <>
month_name <>
" " <>
binary_year <> " " <> padded_hour <> ":" <> padded_minute <> ":" <> padded_second <> " GMT"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It feels the formatter could do a better job here. I will investigate.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, good catch, I noticed this but forgot to point it out. 💟

@josevalim
Copy link
Member

I will merge and tackle Plug.Conn and the other comments dropped by @fertapric!

@josevalim josevalim merged commit 729cbb8 into master Feb 13, 2018
@josevalim
Copy link
Member

❤️ 💚 💙 💛 💜

@josevalim josevalim deleted the al/formatter branch February 13, 2018 08:54
Gazler pushed a commit to Gazler/plug that referenced this pull request Oct 18, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants