makandra / makandra-rubocop

makandra's default Rubocop configuration
MIT License
6 stars 1 forks source link

Space before safe navigation operator #39

Closed codener closed 1 year ago

codener commented 1 year ago

Ruby 2.3 added the safe navigation operator &.. Currently, Rubocop enforces no spaces around it, but I suggest to precede it with a space for readability.

# Current
method&.other

# Suggested
method &.other

The latter is not only easier to parse, the space also indicates that the invocation chain could end there.

Besides easier parsing of the expression, the reason is the source of that operator. a &.b resembles a && a.b, just like a ||= b is a || a = b. By merging || a = to ||=, it becomes a ||= b (spaces kept). The same way for a && a.b: if we merge && a. to &., it becomes a &.b (space between a & kept; between .b has not been a space).

Vote

:+1: Always have a space before &. :rocket: Allow both :-1: Keep current behavior: do not allow a space before &.

dastra-mak commented 1 year ago

That's an interesting line of thought. I agree that it could be helpful. On the other hand that's pretty uncommon, too.

Which cop do you want to change, is it SpaceAroundMethodCallOperator? As I see it there is no easy way to allow your preferred syntax without also allowing spaces around other method calls (like foo . bar, which we don't want to allow). So I vote for keeping it the way it is.

codener commented 1 year ago

You're right, Rubocop does not have an option for this. I would adapt that cop if we decide for the space.

kratob commented 1 year ago

I actually have more difficulty parsing this. At first glance I parse

method &.other

as

method(&.other)

There are very similar looking use-cases where my way of parsing is correct:

render &block

which is

render(&block)

It's possible this only happens for the example, because method is so short.

codener commented 1 year ago

I understand: similarly, method &:other is parsed as method(&:other).

Still these need to be distinguished anyways:

method &&= other
method && other
method &.other
method &:other
method &other

It's characteristic to the "logical &" that it works with or without (preceding) space:

method&&= other
method&& other
method&.other

So why squeeze the & to the method name for safe navigation, when in any other case the & has a preceding space? It's not "space denotes method arguments coming", rather it is "space increases readability". That's why I am proposing this change.

triskweline commented 1 year ago

I also have problems parsing with a leading space.

I also have never seen a space before the & in any documentation.

foobear commented 1 year ago

I concur with @kratob.

Most commonly, spaces are used around operators or when omitting method parenthesis.

I read method&.other as "usually method.other". We would not write that as method .other even though it's syntactically possible. Similarly, method&.other makes more sense to me than method &.other.


It's characteristic to the "logical &" that it works with or without (preceding) space:

Both && and & are operators, hence they should be surrounded by spaces.

codener commented 1 year ago

&. is a Ruby operator as well – one more reason to precede it with a space. Actually, Rubocop notes this as an exception to the rule, which I am making a case against. I would even say it has an "operator side" and a "method invocation side".

Without the space, the & is easily overlooked, despite being crucial to the program flow. Being written right after the method name, it can be mistaken as part of the method name (like the ? is in empty?) on first and second glance, which is amplified by its letter-like shape. Real-world examples (line by line):

predicate?(paragraph&.text)
current_academy_partner&.allow_registration
List.first.elements.first&.name
value = video&.most_value
topic&.to_s || super
if description&.match?(regex)
search_location&.name&.to_s

Remember, without the safe navigation operator, all these examples would be written as some form of a && a.b, e.g. value = video && video.most_value, and value = video &.most_value resembles this very well.


While I believe the evidence is obvious, I acknowledge the syntax is uncommon. Adhering to industry standards is a strong argument. If not accepting the leading space: would we allow both syntaxes (for legibility), or never have a leading space (for consistency)?

foobear commented 1 year ago

You're right, &. is an operator. As is .. :slightly_smiling_face: And we don't put spaces around the . operator but all logical or comparison operators. That is also what affects reading code (for me): spaces mean some kind of comparison or composition, not accessing object properties or methods of objects.

Without the space, the & is easily overlooked

IMO that's the main point of &. -- the nil check is secondary and less important (both "object is nil" and "its method returns nil" are possible outcomes that need to be handled anyway)

despite being crucial to the program flow

In such cases, I'd rather structure code differently, e.g. by using guard clauses. If you really care about a nil check and explicitly want to say a && a.b but RuboCop::Cop::Style::SafeNavigation complains, maybe disable that cop (locally or in the entire project).

I rarely feel the need for a && a.b instead of a&.b any more and am more disturbed by uncommon spacing around &. than not noticing the nil check. Maybe I'm using more guard clauses or just write methods differently. :shrug:

codener commented 1 year ago

Closing this because of voting result. Adhering to industry standards is important.