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

Spaces around parentheses edge case #10

Open
NickLaMuro opened this issue Dec 12, 2013 · 7 comments
Open

Spaces around parentheses edge case #10

NickLaMuro opened this issue Dec 12, 2013 · 7 comments

Comments

@NickLaMuro
Copy link

Rule in question

http://sportngin.github.io/styleguide/ruby.html#source-code-layout_L9

Beef

So in the every day use case:

method(arg_one, arg_two)

I think avoiding the spaces is fine, but when you start nesting them:

method_one(method_two(arg_one), method_three(method_four(arg_two)))

It becomes a bit less legible. While the above example is a bit contrived, and probably breaks the law of demeter in multiple ways, there are use cases where this happens, or makes sense to happen. Scopes come to mind:

scope :has_field { joins(:other_table).merge( OtherTable.different_scope ) }

Where I think it is beneficial for readability (IMHO). All the examples from the style guides we are taking notes from don't really make a distinction, and just have a hard rule of "don't do this". I (personally) think we should make the exception in this case.

Discuss.

@anfleene
Copy link
Contributor

I'm meh on this. I don't think either look all that great. I think too many nested parenthesis are sign you need to refactor.

@anfleene
Copy link
Contributor

I personally don't like scopes if you changed the example to:

def self.has_field
  joins(:other_table).merge(OtherTable.different_scope)
end

you avoid it all together

@NickLaMuro
Copy link
Author

@anfleene in the case there different_scope takes a param that you are passing in, your argument does nothing to prevent that:

scope :has_field {|value| joins(:other_table).merge(OtherTable.different_scope(value)) }

V.S.

def self.has_field(value)
  joins(:other_table).merge(OtherTable.different_scope(value))
end

Regardless of using your suggestion or not, .merge(OtherTable.different_scope(value)) still causes the )) to happen. The only "refactoring" argument you could make here is move that into a local variable, only to just pass it in right away and not use it again.

If it wasn't clear in the description, I am not thinking the change should be to enforce this, but just lift the "hard requirement" in the style guide.

@anfleene
Copy link
Contributor

ah yeah I agree with you then. There are places where adding spaces to parenthesis does make it more readable.

@jphenow
Copy link

jphenow commented Dec 17, 2013

Aaah, your reasoning makes more sense to me now, I thought you just felt like being able to add extra spaces whenever. None the less, I have a hard time with ever doing extra spaces. This seems like a place where adding the optionality just makes the rule more unnecessarily cumbersome. I'm of the feeling that the styleguide should be as strict as possible, except when there are special cases that you actually have to allow for (exceptions in the style so that the code will function correctly).

tl;dr
I disagree that this makes it much more visibly pleasing, but I can live with making it an option.

@carlallen
Copy link
Contributor

I actually find adding all the extra spacing around parens harder to read. I also agree with @anfleene , if you have this problem really bad it's a good time to refactor.

Also if it's really bad, you can split the method call onto multiple lines, which will give a much cleaner and more readable chain anyway

@anfleene
Copy link
Contributor

@carlallen totally agree 👍

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

No branches or pull requests

4 participants