Skip to content
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

Formatting: add white space after comma #4491

Merged
merged 1 commit into from
Apr 18, 2016

Conversation

eksperimental
Copy link
Contributor

Standardizes the use of comma leaving a white space after it whenever applicable.

Note #1: In the changed lines it also adds a white space surrounding the | when there is more than one element to the left.
Example: before was [a,b|tail] now it is: [a, b | tail].

.Note #2: It does not enforce this in quantifiers in regular expressions such as in: x{1,3}

@@ -735,7 +735,7 @@ defmodule KernelTest do

test "multi-call" do
assert capture_io(fn ->
Code.eval_string("use UseMacro.{SampleA, SampleB,}", [], __ENV__)
Code.eval_string("use UseMacro.{SampleA, SampleB, }", [], __ENV__)
Copy link
Member

Choose a reason for hiding this comment

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

✂️

@josevalim
Copy link
Member

Thank you @eksperimental. @ericmj has already left some comments.

@lexmag, I will leave this one up to you. :)

❤️

@whatyouhide
Copy link
Member

I'm not understanding why we're putting spaces around | only if its LHS has two or more elements. I'd leave it without spaces as far as this PR goes, and maybe open a new PR that standardizes the use of | by always putting spaces around it?

@eksperimental
Copy link
Contributor Author

@whatyouhide: in short because
the original version [?.,{?*,?$}|tail] would look cleaner than the one with only space after comma [?., {?*, ?$}|tail]

Creating a PR that puts a white-space around the | will be a daunting task (1400+ occurrences) and is PR that may likely not get approved.

plus the use | surrounded by white-spaces almost equals the use of | not surrounded by white-spaces.
I think it's 4 lines in total.

@whatyouhide
Copy link
Member

So maybe adding spaces around all |s would be a daunting task, but then let's not add the spaces for the occurrences with [a, b|c] either? Wdyt?

@eksperimental
Copy link
Contributor Author

I can revert those. no problem

@lexmag
Copy link
Member

lexmag commented Apr 8, 2016

We agreed on having spaces around | – it simply improves readability (especially for some fonts that have super thin vertical line), though we probably shouldn't fix it in one go but rather do when we touch such code (I hope in 10 years we will fix that :)).
@eksperimental you can keep spaces around | since these lines already affected.

end

test "evaluates with defined variable" do
assert_eval "foo 1", "foo <% bar = 1 %><%= bar %>"
end

test "evaluates with require code" do
assert_eval "foo 1,2,3", "foo <% require Enum, as: E %><%= E.join [1, 2, 3], \",\" %>"
assert_eval "foo 1, 2, 3", "foo <% require Enum, as: E %><%= E.join [1, 2, 3], \", \" %>"
Copy link
Member

@lexmag lexmag Apr 17, 2016

Choose a reason for hiding this comment

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

Let's keep previous formatting here as it is not a code but data.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

@lexmag
Copy link
Member

lexmag commented Apr 17, 2016

@eksperimental I've just added very last comment, LGTM.

@eksperimental
Copy link
Contributor Author

@lexmag regarding the changes with spaces surrounding |, if we are planning on merging #4507, it's better to remove them from this commit (changes and commit msg).

@lexmag
Copy link
Member

lexmag commented Apr 18, 2016

@eksperimental right, let's preserve | changes for that PR. :bowtie:

@eksperimental
Copy link
Contributor Author

@lexmag we should be good to go
I will be rebasing #4507 on this commit

@@ -25,22 +25,22 @@ defmodule FileTest do
#
# Renaming files
# :ok -> rename file to existing file default behaviour
# {:error,:eisdir} -> rename file to existing empty dir
# {:error,:eisdir} -> rename file to existing non empty dir
# {:error, :eisdir} -> rename file to existing empty dir
Copy link
Member

Choose a reason for hiding this comment

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

These arrows have gotten broken indentation.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed under "Renaming files" and "Renaming dirs" sections

Standardizes the use of comma leaving a white space after it whenever applicable.

Note: It does not enforce this in quantifiers in regular expressions such as in: `x{1,3}`
@lexmag lexmag merged commit bed9816 into elixir-lang:master Apr 18, 2016
@lexmag
Copy link
Member

lexmag commented Apr 18, 2016

💛

@eksperimental eksperimental deleted the commas branch April 18, 2016 23:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants