Implemented Layout/SpaceInsideParens with EnforcedStyle: compact

[itay-grudev]

Layout/SpaceInsideParens:
  EnforcedStyle: compact

@example 
# The `compact` style enforces that parentheses have a space at the
# beginning with the exception that successive right parentheses are allowed.
# Note: Empty parentheses should not have spaces.

# bad
f(3)
g = (a + 3)
y( )
g( f( x ) )
g( f( x( 3 ) ), 5 )

# good
f( 3 )
g = ( a + 3 )
y()
g( f( x ))
g( f( x( 3 )), 5 )

---

Before submitting the PR make sure the following are checked:

• The PR relates to only one subject with a clear title and description in grammatically correct, complete sentences.
• Wrote good commit messages.
• Commit message starts with [Fix #issue-number] (if the related issue exists).
• Feature branch is up-to-date with master (if not - rebase it).
• Squashed related commits together.
• Added tests.
• Ran bundle exec rake default. It executes all tests and runs RuboCop on its own code.
• Added an entry (file) to the changelog folder named {change_type}_{change_description}.md if the new code introduces user-observable changes. See changelog entry format for details.

[dvandersluis]

I'm not sure I understand why it'd be desired to compact only right parens? If this is a style that people use, should it not apply on both sides? Or else, the style should probably be called compact_right or something similar. I'm not sure if "compact" even makes sense here when most of the parentheses have white space around them, which makes them not compact 😄 Maybe collapse/collapse_right (my preference would for this to be symmetrical if we were going to add this style).

Also you'd need to update VersionChanged in default.yml.

[itay-grudev]

The code style we enforce at Clustermarket is a bit odd, although everyone approves it. The idea is put spaces around important words, and to decrease the emphasis (by decreasing whitespace) around language grammar, which is always there and is expected. Long story short we believe it improves readability and allows us to scan through code diagonally faster.

Also compact left doesn't really make sense, because you don't really do: ((( something ) + 2 ) + 4 ) * 2.

[dvandersluis]

I'm not convinced that this is a style that makes sense for RuboCop as a whole. You could implement it as a custom cop for your codebase/company and just disable Layout/SpaceInsideParens.

@bbatsov what do you think?

[itay-grudev]

RubCop is a collection of rules, configurable to suit different projects and different teams with different views. The objective of the project is internal consistency, with a healthy set of defaults. If there was one style that was suitable for everyone - there wouldn't be any need for configuration. But that's not the case and there is configuration.

All is good until you miss the one option you need, in which case you either disable that corresponding cop, or if you want to be better than that - you spend the time to reverse-engineer how Rubocop works and modify it to include your use case. And I did exactly that, I wrote tests, documentation, and followed your particular style and recommendations as close as I can.

Saying that this style doesn't makes sense for RuboCop as a whole, kind of violates the spirit of Rubocop, doesn't it? It adds too much personal opinion, in a project where personal opinion should be no more than a default, albeit configurable option.

[dvandersluis]

@itay-grudev I'm not suggesting that there should only be a single allowed style -- in fact this cop is already configurable as you have seen, and we support opposing styles on many cops. That being said, we cannot guarantee that every single style that a company uses makes sense for the RuboCop userbase as a whole. I'm also not saying that this definitely doesn't fit, I'm interested what others on the team think.

We often recommend niche rules be implemented as custom cops (in fact, we have our own custom cops at my workplace that don't necessarily make sense for the tool as a whole). It is simple to create your own cop, and the effort you have put in for this PR would not go to waste, because it could be repurposed as such.

[bbatsov]

Indeed. We can't possibly support any style that exists (that would mean an epic overhead in the RuboCop codebase), that's why we focused only on those that are common within the community. I'm leaning towards rejecting the proposed addition.

[itay-grudev]

@bbatsov @dvandersluis We have compact for the following:

Layout/SpaceInsideArrayLiteralBrackets

array = [ a, [ b, c ]]
array = [[ a ],
  [ b, c ]]

Layout/SpaceInsideHashLiteralBraces

h = { a: { b: 2 }}
foo = {{ a: 1 } => { b: { c: 2 }}}

The Layout/SpaceInsideParens compact version just adds consistency.

g( f( x ))
g( f( x( 3 )), 5 )

Also I plan to write an article about why this may be better from a neuroscience perspective and information theory perspective.

[bbatsov]

Hmm, I don't recall when those were added. I'll have to dig up the relevant tickets.

The Layout/SpaceInsideParens compact version just adds consistency.

I guess now I'm more receptive to accepting this, but I first have to recall what exactly was I thinking back in the day for those other cops. Perhaps back then I still wasn't as afraid of complexity. :D

[bbatsov]

Looking at this ticket things started from the SpaceInsideHashLiteralBraces cop and were just copied everywhere else. More digging is needed!

[itay-grudev]

@bbatsov In any way - while not as popular, this pattern actually improves readability. We put spaces around content important for humans. Here is an excerpt from our internal guidelines:

Numerous neuroscience researchers showed that the way the human brain processes text is by looking at a words as a whole rather than individual letters. By improving contrast between different words we help our brain read faster. For example, reading the following sentence is significantly harder when written like this:

Whitespace(is)oneofthe$most%important#tools@in!our+arsenal;to/increase>code>readability, and it all comes to increasing contrast between different words in a sentence. For example, code highlighting was invented just for that purpose. And while highlighting is usually sufficient to add contrast, adding whitespace correctly further improves it. By employing both syntax highlighting and correct whitespace usage we can help our brains to read code diagonally much faster.

So the real question then is - where do we add whitespace? What do we add contrast to? What is important?

Like every language, programming languages have grammar. Grammar is important for compilers or interpreters to understand our code correctly, but it's not that important for humans. We can still understand a sentence with a grammatical error, while a computer won't. For this reason, grammar in a programming language is more important for computers than for humans for reading and understanding a piece of code, hence why grammar is considered low priority when it comes to contrast.

Instead we should put contrast to part of our code that have meaning to us - like method and variable names, expression operators, values, etc. and not brackets, flow control operators, etc.

Across the app (Front-end and Back-end), we believe spaces are key to code readability. Think of your code as important words or tokens with some grammar in-between. The main rule is that grammar is not as important as content and we add spaces to highlight words, not grammatical structures. Consider the following generic (not necessarily Ruby example):

#   ╔ language grammar (surrounded by spaces, but note - the whole expression including the opening bracket)
#   ║       ╔ variable name (surrounded by spaces)
#   ║       ║    ╔ expression operator (surrounded by spaces as it is important for understanding the context of the expression)
#   ║       ║    ║       ╔ string value (surrounded by spaces)
#   ▽       ▽    ▽       ▽
    if( variable == "expression" ){
#     △                           △
#     ╚ a bracket by itself is not a word and adds very little meaning. It's just part of the language's grammar, it is expected there - so no spaces.

# Bad
CollectionProcesser.new( Model.where( nested_model_name: { condition: value, condition_2: value } ) )

# Good
CollectionProcesser.new( Model.where( nested_model_name: { condition: value, condition_2: value }))

[flanger001]

RubCop is a collection of rules, configurable to suit different projects and different teams with different views.

Exactly, and it is possible to write custom cops for your team for exactly this reason. People submit custom cops here all the time. Sometimes they get added, sometimes they don't.

[bbatsov]

I'd suggest taking a look here https://docs.rubocop.org/rubocop/1.19/index.html#philosophy

The early feedback we got lead us to adopt of philosophy of (extreme) configurability and flexibility, and trying to account for every common style of programming in Ruby.

"common" is the key word here. Anyways, I'll admit I've not always thought enough about some of the things I've accepted, plus over time, as the project grew I became significantly more concerned about the overhead of maintaining some niche styles.

Again, I'm not rejecting this particular proposal (I'll likely merge it if this has existed in similar cops), I just want everyone to know what we're aiming for in general.

[koic]

This is a perspective on the keyword "common". IMHO, I've never encountered this style in well-known programming books and OSS of Ruby. (I may have seen it in Perl...?, but I don't remember it well.) Of course this is my personal perspective. I know that each programmer encounters different styles.

OTOH, I agree with @dvandersluis as a maintainer. I also have several niche custom cops at my workplace.

[bbatsov]

@koic I think we're all in agreement here, but as @itay-grudev pointed out somehow this style got added to two other existing cops (Layout/SpaceInsideArrayLiteralBrackets and Layout/SpaceInsideHashLiteralBraces). For the sake of consistency we either have to add it here as well or remove/deprecate it from the other cops. I have no idea why we felt those braces needed special handling in literals, but not in general, back in the day.

[koic]

@bbatsov Thank you for your explanation. My understanding has caught up. But this is certainly a difficult issue to choose. I haven't come up with a better answer yet.
This is a maintenance perspective. The compact style may be relatively accepted if the alternative option doesn't increase in the future. However, the tendency cannot be confirmed.

[bbatsov]

True. At some point we'll have to evaluate all cops/supported styles and remove uncommon stuff to improve the long-term maintainability of the project.

[dvandersluis]

I just want to point out again that the style on the other cops are symmetrical (applies to both left and right parens) whereas this one is not (only applying to right parens). If we add this I still think it should also be symmetrical to match?

[itay-grudev]

@dvandersluis Well, if you agree to merge it then I'll spend the extra time to make those changes and test them, so it's consistent with the other two cops.

[dvandersluis]

Up to @bbatsov if we want to continue with this or deprecate the style from the other cops I guess.

[bbatsov]

I've decided to continue with adding compact here, as the less disruptive change for the users. And to be more careful about the styles we accept going forward as well. :D

[dvandersluis]

Alright @itay-grudev please update to make this symmetrical then!

[itay-grudev]

@dvandersluis I will finish it later this week and I will ping you when it's ready. Thanks a lot guys!

[itay-grudev]

@dvandersluis Implemented left side compaction. I also moved the correction code inside each respective style processor because I started tripping Metrics/MethodLength and needed to shorten the code.

[bbatsov]

Looks good, apart from my small remark.