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
Hanging Indentation on methods #359
Comments
👍 |
👍 as well. The combination of meaningfully-long class/method name plus meaningfully-long hash-parameter keys makes the current standard jarring and unwieldy. I don't recall having seen a similar convention in any other language I've ever worked in. Our shop standard allows for doubly-indented continuation lines, which would render @baweaver's example as # Alternate line-continuation standard demonstration
some_method.with.a_longer_chain :parameter_1,
:parameter_2,
:parameter_3,
:parameter_4
# more code... The other feature of this is, that by using a double indent for continuation, you don't stop as you're browsing down the code hunting for the conditional that isn't there. By having the first parameter on the first line, you get the reminder from the trailing comma that what's below really is continuation. |
Yes. But I think we already have similar rules in the guide. |
@baweaver Another drawback of this is if you refactor the method name,
@jdickey There is one in scheme (+ 1
(foo 3.5 a)
bar
baz)
@jdickey What about using one level of indentation? some_method.with.a_longer_chain :parameter_1,
:parameter_2,
:parameter_3,
:parameter_4
# An empty line above more code...
@bbatsov That rules does not need to consider indentation. |
@weakish The reason we prefer two levels of indentation to one in that usage is to avoid confusion with a block, such as for a conditional. With one level of indentation, you'd be skimming down the listing, see things go one level in, and go back and look for a conditional or such that you missed the first time. "Don't overload the meaning of a single indent", is how one dev put it. I can live with the single indent; it's just not so immediately clear what it means. |
Based off the example @bbatsov linked to in September, I would assume either a hanging indent or a normal indent would be acceptable. I've predominantly used hanging indents, is there an argument towards one or the other, or should both be acceptable uses? |
The problem with hanging indentation is that it's annoyingly difficult to read with longer methods, especially if keyword arguments are used. |
That is true, and I think that the example you gave doesn't adequately illustrate this. Either reducing the line length or dropping to a new line should address this issue… How about this? # Bad - Hanging Indentation leads to multiple long lines
very_long_kumquats = some_method.with.a_much_much_much_much_much.longer.chain(:parameter_1,
:parameter_2,
:parameter_3,
:parameter_4)
# Good - Use method chaining to reduce line length
very_long_kumquats = some_method.with.
a_much_much_much_much_much.
longer.
chain(:parameter_1,
:parameter_2,
:parameter_3,
:parameter_4)
# Good - Drop to a new line
very_long_kumquats = some_method.with.a_much_much_much_much_much.longer.chain(
:parameter_1,
:parameter_2,
:parameter_3,
:parameter_4
) |
I'd lean towards the third myself, though with multiple chains on there I'd likely go through to: very_long_kumquats =
some_method
.with
.a_much_much_much_much_much
.longer
.chain(
:parameter_1,
:parameter_2,
:parameter_3,
:parameter_4
) Though at that point you should likely just compose those into another method to give intention to what you're doing with that long of a chain. Really, if I end up chaining over 5 items I try and break them into named intention revealing methods, otherwise I end up with something that's really annoying to tell exactly what I did and why I did it at a later date.
In rails at least it's very very easy to have massive methods in which you have something that looks like this: @m = Model.where(foo: bar, biz: baz, fizz:buzz).pluck(:a, :b, :c).map { |(a,b,c)| do_something }.select(&:itself).group_by(&:itself).map { |k,v| [k, v.length] }.to_h That's pesky to know I'm getting records which match those conditions, extracting values, transforming them, and counting by that. At that point just break out a new method: def self.get_fizz_buzz_abcs
where(foo: bar, biz: baz, fizz:buzz).pluck(:a, :b, :c)
end
def self.transformed_abcs
get_fizz_buzz_abcs.map { |(a,b,c)| do_something }.select(&:itself)
end
def self.counts_by_abc
transformed_abcs.group_by(&:itself).map { |k,v| [k, v.length] }.to_h
end Contrived, yes, but general rule of thumb I want to convey there is to not make massive chains unless necessary. After a while it becomes very hard to manage, FP style or not. |
Care to make a PR so @bbatsov can really review this? |
I can get to it a little later tonight. Probably on both counts. |
Just imagine that you want to rename Eveything where indentation depends on the length of identifiers of the previous line is unmaintainable due to the identifiers' length inconstancy in the development cycle. This:
and this only. |
Based off of a very old issue that had a lot of interesting discussion: rubocop#359 The idea here is that hanging indentation is incredibly hard to read while going down a page, and also incurs extra editing whenever more methods are added to a chain.
Based off of a very old issue that had a lot of interesting discussion: rubocop#359 The idea here is that hanging indentation is incredibly hard to read while going down a page, and also incurs extra editing whenever more methods are added to a chain.
Yes, please! |
Another case where "No Hanging Indentation" syntax might be better is where you need to remove a single argument from a list. You only need to remove a single line of code which will produce cleaner git history log. While if you're using Hanging Indentation, you'd need to touch other lines as well when you're modifying first or last argument.
|
Just one thing to note here. There's two separate things, I think:
For example, the following are examples of valid hanging and no hanging indentation, but with multiple parameters per line. # With Hanging Indentation
some_method.with.a_longer.chain(:parameter_1, :parameter_2,
:parameter_3, :parameter_4)
# Without Hanging Indentation
some_method.with.a_longer.chain(
:parameter_1, :parameter_2,
:parameter_3, :parameter_4
) This style is actually the most common in the ruby community, and it does not help with cleaner diffs at all. I made a feature request to rubocop a while ago to enforce one parameter per line, but it does not seem very popular. |
I'd like to propose an addendum. Often times when one has a longer method, you have to do something with the parameters to get the method under 120 lines. One method of doing this is hanging indentation, which is very distracting when trying to grok a larger code base. It breaks the flow of reading.
Now granted the example is mildly confounded and you shouldn't chain methods that long. That being said, it's not unusual to see hanging indentations on 1-2 method chains past the 80th column which makes it near impossible to quickly skim over things.
Can we consider adding this to the guide?
The text was updated successfully, but these errors were encountered: