Skip to content

Formmating: Add white space around vertical bar #4507

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 2 commits into from
Apr 24, 2016

Conversation

eksperimental
Copy link
Contributor

It adds a white space around the vertical bar, improving readability of the code.
Carefully edited, and thoroughly reviewed three times.

While the amount of lines edited is significant, I think it's a great improvement in the clarity of the code.

@josevalim
Copy link
Member

@lexmag it is your call. :)

@ericmj
Copy link
Member

ericmj commented Apr 11, 2016

I have no strong opinion on it but personally I am not convinced it's an improvement.

@whatyouhide
Copy link
Member

It's a huge amount of changes and we'll be blaming @eksperimental for years in git :P, but maybe this is an improvement after all (consistency is good). We'd have to pay a lot of attention when reviewing PRs so that we don't reintroduce x|y though.

@@ -943,8 +943,8 @@ defmodule Enum do
end) |> :lists.reverse()

case list do
[] -> []
[_|t] -> t # Head is a superfluous intersperser element
[] -> []
Copy link
Member

Choose a reason for hiding this comment

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

Let's fix this the other way around to [] -> [].

@eksperimental
Copy link
Contributor Author

@lexmag all fixed, and a few more than i found with a search command. I looked for changes in line containing -> but i overlooked the ones with do:, so thanks for that!

the CI build failed, but I think it's unrelated, since locally it just passed

@vikram7
Copy link

vikram7 commented Apr 15, 2016

I think this is a pretty good improvement for readability. Of the handful of Elixir docs that I've read through (like here), [one | two] seems to be more common than [one|two]. If that's the case, then it would be worth making sure they match up. 😄

@eksperimental
Copy link
Contributor Author

@lexmag: rebased to master

whatyouhide added a commit to whatyouhide/redix that referenced this pull request Apr 21, 2016
@lexmag lexmag merged commit 4ceb41e into elixir-lang:master Apr 24, 2016
@lexmag
Copy link
Member

lexmag commented Apr 24, 2016

@eksperimental 💛

@lexmag
Copy link
Member

lexmag commented Apr 24, 2016

Seems GitHub has fixed the whitespace ignoring feature ?w=1, with it we have 0 files changed. 😄

@eksperimental eksperimental deleted the space_vertical_bar branch April 24, 2016 23:03
@eksperimental
Copy link
Contributor Author

eksperimental commented Apr 24, 2016

@lexmag sweet!
thanks for merging.. I didn't know about that cool feature

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.

6 participants