Rule change suggestions

[keithrbennett]

Here are some thoughts I had when I recently ran standardrb --fix on a code base. They are not equal in importance to me, but I mention them all anyway. There's some personal preference here, but there are a lot of objective observations as well. For example, I mention some concepts of user interface design that I believe we as developers are entitled to expect, or at least be permitted to write, in the source code with which we work. In general, I'd love to see more application of these user interface design principles and less of "what I'm used to" in the formation of standards to which we will all be required to conform if we work on a project that uses this tool.

Horizontal Alignment of Values and Blocks

This rule violates basic user interface design principles. Horizontally aligning the values allows the reader to spot the values with a minimum of eye movement and effort. There are many instances in code of repeating sets of values and it can be useful to easily scan them. Would any of us ever implement text fields in a web form in this way, where labels are horizontally aligned, but the input fields are horizontally scattered? Why aren't we developers worthy of the same consideration as our users? I understand not requiring alignment, but why prohibit it?

And vs. &&

I realize that and is often used erroneously, but the prevailing wisdom, as I understand it, is that &&/|| should be used for logical operations but that and/or should be used for flow control (see Avdi Grimm's article at https://avdi.codes/using-and-and-or-in-ruby/). So and/or should be permitted. Also, the two pairs differ in precedence rules; I wonder if this could result in runtime errors in some cases as well.

Parenthesizing Ternary Conditions

It is often helpful to the reader to wrap a ternary expression's condition in in parentheses, to explicitly show its beginning and end. This is especially true when the condition is more complex than the one in this example. I suggest permitting parentheses.

Method Parameter Indentation

The example in this image is not so bad, but I've seen cases where horizontal space is limited, the method name is longer, and this formatting style pushes parameters way to the right, sometimes out of view. Does standardrb treat long lines the same as the ones in this example?

Use of ".()" as an Abbreviation for ".call("

Callables are objects that respond to a call method, and can include Proc's (lambdas and non-lambda procs), and other objects, i.e. instances of other classes that define a call method. Procs/lambdas are first class objects in Ruby, and the addition to the language of the stabby lambda and .() notations were nods to the importance of functional programming in Ruby. Prohibiting the use of this abbreviation undoes a Ruby feature that was put there to help us. I realize that it is unfamiliar at first, but the mental translation of .() to .call() is a small jump. Using .call seems innocuous when it is only occasional, but as it is used more, the verbosity becomes distracting and annoying.

Replacing do/end with {}

This was the most surprising one to me. I would have expected the reverse to be enforced, since using do/end for multiline blocks has been a convention for many years now.

Prohibiting Spaces for {}

This one was surprising to me too. I suppose one could justify it by saying we do the same thing with parentheses in method calls, but it's a very uncommon pattern, at least in my experience. If we are to ask developers to break old habits to learn new ones, I think it should be for issues with more objective usability merits than this one.

Enforcing Double Quotes

There have been many times when I have had to input many quoted strings, and I've long appreciated being able to avoid the Shift key when doing so. In addition, I find single quotes more readable (i.e. it is easier to visually distinguish between the quote marks and the text they delimit), especially when there are many of them. It would be nice to continue to have the option to use single quotes.

Expression Continuation Indentation

A somewhat related issue is that this single-indentation-level rule makes it impossible to distinguish (by spacing at least) expression continuations from changes in nesting levels. A convention that has been used by many, for many years, is continuation indentation, which is typically two indentation levels (as shown on the left). My editor, RubyMine, has a separate setting for continuation indents, and possibly many others do as well.

It helps because when scanning the code one can easily spot the horizontal distance between left margins of the first and subsequent lines, and immediately know, before even looking at any text, whether it is a change in nesting level or an expression continuation. This communicates information to the reader that would otherwise not be available before mentally parsing the code itself.

This image is not a great example but was the only one I noticed; a better one would have line continuation and nesting level changes in the same block of code.

[itsliamegan]

A few clarifications while reading these that might be useful for @searls or others:

Method Parameter Indentation

I think this is the overlapping area of a few config options. First is Layout/ArgumentAlignment which is currently configured to enforce only using fixed levels of indentation for multiline arguments. Since you're using keyword arguments, which Rubocop interprets as a hash, the Layout/HashAlignment option kicks in. The configuration for this rule says (essentially) to always align hash keys so that they start at the same character. Since your first key starts in the middle of the line, that's where it aligns. I'm not sure what the best behavior is in this case. If you move the first argument to the next line, the keyword arguments will all align one indentation level in, which is my personal preference.

Replacing do/end with {}

In this scenario, I think this is an error. The current rule is using a strategy called "semantic blocks". This means using do/end for imperative blocks with side effects and {} blocks for functional blocks, like those that you'd pass to Enumerable#filter or Enumerable#map. In this case, since the block passed to find_or_create_by! is imperative, this is probably just a matter of adding more Rails methods to Standard/SemanticBlocks/ProceduralMethods. I'm happy to open a PR adding some more Rails methods to this list!

[keithrbennett]

@itsliamegan thanks for your helpful comments. I don't know much about the Rubocop rules, and I'm sure there are reasons for at least some of standardrb's choices of which I'm not aware and can learn from.

Method Parameter Indentation

If I do this:

def foo(
  a,
  b)
  # ...
end

I get this when running standardrb:

x.rb:3:4: Layout/MultilineMethodDefinitionBraceLayout: Closing method definition brace must be on the line after the last parameter when opening brace is on a separate line from the first parameter.

This, however, is permitted:

def foo(
  a,
  b
)
  # ...
end

This is mathematically and logically sound, and I kind of like it.

Replacing do/end with {}

I had seen mention of semantic blocks over the years, but it didn't occur to me that's what was going on.

I like the idea of semantic blocks in principle, but when do end are on a single line it looks weird to me. Honestly, the more I read about this the more I think I would prefer using curly braces all the time as in C, C++, and Java, if I remember correctly; but I don't expect anyone to agree with me on that. ;)

I've never used semantic blocks; my questions to those who have:

1. how often does it happen that both functional and procedural things are happening in a block?
2. Do developers, especially new developers, have a hard time figuring out which one to use?

[jmkoni]

Horizontal Alignment of Values and Blocks

I actually disagree with your points here. No, we would not design a web form in this way, but we would also not design a web form that looks like the code on the left. That causes different issues… it makes it more challenging to tell which value maps to which key (espeically if you have some very short keys and some very long keys), it causes git churn (the spacing changes for every line when you add a longer key), and it’s harder for new developers (if you don’t have your editor set up that way, you are trying to fix it manually). (just for reference because I keep forgetting, this rule is Layout/HashAlignment)

And vs. &&

While this particular case is valid, most cases require the use of &&. This case is rare enough that it’s easy enough for the experienced developer doing that to just add a # rubocop:disable Style/AndOr after it. In this particular example, I’m skeptical that the and return is even needed at all. See past conversation on this here: #73

Parenthesizing Ternary Conditions

I think that, when you get to the point where your ternary is hard to read, you should maybe consider not using a ternary. (again, for reference, this rule is Style/TernaryParentheses)

Method Parameter Indentation

TBH I am not sure which rule is causing this. We use with_fixed_indentation for Layout/ArgumentAlignment, but this seems to be doing the opposite, so it must be another rule. NOTE: it is! It's thinking kwargs is a hash, so if you move the first argument to the next line, this will look the way you expect. SECOND NOTE: created #216.

Use of ".()" as an Abbreviation for ".call("

I’m going to again strongly disagree here. That is a big mental jump if it’s unfamiliar. And calling 4 characters verbose is a bit of a stretch. And this isn’t just us… it’s also the rubocop default as well https://github.com/rubocop-hq/ruby-style-guide#proc-call/

Replacing do/end with {}

You can see some of the conversation in the past here (#109). This is our own custom SemanticBlock rule.

Prohibiting Spaces for {}

For background, we decided to eliminate spaces for hash literals and require spaces for blocks to make it easier to tell the difference between the two at a glance. See #26 cop: Layout/SpaceInsideHashLiteralBraces (note, previously made a mistake and thought this was Layout/SpaceInsideBlockBraces)

Enforcing Double Quotes

The point of standard is consistency, not options. Upside, you can type out all of your strings as single quotes and standard will autocorrect. Easypeasy. It's definitely easier for most people than having to remember when you need double quotes otherwise.

Expression Continuation Indentation

I am not following what the issue is here at all.

[jmkoni]

Opened #216 and working on a fix for that.