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

Wanted: Layout/SpaceInsideArrayLiteralBrackets #4811

Closed
LandonSchropp opened this issue Sep 29, 2017 · 10 comments
Closed

Wanted: Layout/SpaceInsideArrayLiteralBrackets #4811

LandonSchropp opened this issue Sep 29, 2017 · 10 comments

Comments

@LandonSchropp
Copy link

LandonSchropp commented Sep 29, 2017

#I would like to be enforce a Ruby array spacing style like this:

array = [ 1, 2, 3, 4 ]
array = [ "hello", "hola", "bonjour" ]

I've searched the documentation and GitHub issues for something like this, but I haven't been able to find anything. Rubocop includes Layout/SpaceInsideBrackets, but that seems to only enforce removing spaces, not including them. Furthermore, I only want to include spaces inside arrays, not accessor brackets or other bracket operators.

This would be the array equivilent of Layout/SpaceInsideHashLiteralBraces. It's also equivilent to array-bracket-spacing in ESLint.

Rubocop version: 0.50.0 (using Parser 2.4.0.0, running on ruby 2.4.2 x86_64-darwin17)

@mvastola
Copy link

mvastola commented Oct 1, 2017

I would like to +1 this. Having spaces really improves readability IMHO, if you're listing elements.
Going off the convention rubocop seems employ though, I imagine this wouldn't be it's own layout cop but rather a couple of SupportedStyles for the existing cop.

@Drenmi
Copy link
Collaborator

Drenmi commented Oct 1, 2017

Fair enough. How would you like to treat multiline arrays under the new setting?

@mvastola
Copy link

mvastola commented Oct 1, 2017

Hm. I personally use the style:

arr = [
  :a,
  :b,
  :c
]

So this wouldn't apply to me, but I would imagine if you added spaces inside brackets you would do so everywhere. (Including the first/last line of a multi-line array if there is an element on it.)

I have trouble imagining someone constructing any other sort of convention, though if others actually employ such a coding style or else envision it actually being used, then perhaps it merits its own SupportedStyle.

@LandonSchropp
Copy link
Author

Ditto to the multi-line style @mvastola mentioned. I think the array brackets should be configurable in the same way that hashes are. IMO, each one of the following cops should have an array equivalent:

  • Layout/MultilineHashBraceLayoutLayout/MultilineArrayBraceLayout (exists)
  • Layout/SpaceInsideHashLiteralBracesLayout/SpaceInsideArrayLiteralBrackets (doesn't exist)
  • Layout/IndentHashLayout/IndentArray (exists)

So the behavior and supported styles for Layout/SpaceInsideArrayLiteralBrackets should match Layout/SpaceInsideHashLiteralBraces, which would mean these options:

  • EnforcedStyles: no_space (this is already the default)
  • SupportedStyles: space, no_space, compact
  • EnforcedStyleForEmptyBrackets: no_space
  • SupportedStylesForEmptyBrackets: space, no_space

@mvastola
Copy link

mvastola commented Oct 2, 2017

I think I'm mildly against the idea of creating a second cop. If we did this then Layout/SpaceInsideBrackets would definitely need to be renamed and the old name deprecated though, because that cop then would only control certain brackets.

(Would it be SpaceInsideMethodBrackets? SpaceInsideAccessorBrackets? SpaceInsideHashlikeBrackets? Not sure what the best word is.)

I guess it depends on how adverse this project is toward making breaking changes.

I'm not sure what compact does, but it seems this could all be done inside Layout/SpaceInsideBrackets. The SupportedStyles would be: never (current default), all_literals and non_empty_literals.

@LandonSchropp
Copy link
Author

I'm not sure what compact does either. I didn't see any specific documentation for it, but I stuck it in there because it was on the other cop.

I'm not clear on the difference between all_literals and non_empty_literals. Could you please elaborate a little more?

For my use case, my goal is to have space inside arrays, but not inside other brackets.

# Yes
array = [ 1, 2, 3, 4 ]

# No
array[ 0 ] = 1

It looks like there are currently two braces cops: SpaceInsideHashLiteralBraces and SpaceInsideBlockBraces. To me, it makes sense that brackets would work in a similar way. You could have something like SpaceInsideArrayLiteralBrackets for array literals and SpaceInsideReferenceBrackets for brackets used as methods on objects. I'm not quite sure what the brackets object[1] should be called, but the Ruby documentation for Array, Hash and MatchData refers to them as reference methods.

Are there any other uses for brackets that I'm missing in Ruby?

I agree that it's a bummer that this approach would mean deprecating a cop, but IMHO it's worth it if the new properties are explicit.

@LandonSchropp
Copy link
Author

I forgot to mention, there's also SpaceInsideStringInterpolation, which uses braces, but that's obviously a different thing.

@mvastola
Copy link

mvastola commented Oct 2, 2017

all_literals would include empty arrays, whereas non_empty_literals (as the name indicates) wouldn't. So all_literals would specify

arr = [ ]
arr = [ 1, 2, 3 ]

Whereas non_empty_literals would specify:

arr = []
arr = [ 1, 2, 3 ]

The (I think safe) assumption is no one is ever going to want a space only when there's an empty array.

@LandonSchropp
Copy link
Author

Gotcha. I think that's a safe assumption.

garettarrowood added a commit to garettarrowood/rubocop that referenced this issue Dec 8, 2017
The result of removing Style/SpaceInsideBrackets meant that the SpaceInside mixin was only getting utilized in Style/SpaceInsideParens. So this commit includes refactoring the mixin away and moving the necessary logic directly into Style/SpaceInsideParens. In addition, this commit renames the SurroundingSpace mixin's space_between? to space_after?, which is a more accurate description. See comment for more details.
garettarrowood added a commit to garettarrowood/rubocop that referenced this issue Dec 8, 2017
garettarrowood added a commit to garettarrowood/rubocop that referenced this issue Dec 8, 2017
In addition to adding Layout/SpaceInsideArrayLiteralBrackets, this commit breaks out reusable logic from Style/SpaceInsideReferenceBrackets and moves it into the SurroundingSpace mixin.
bbatsov pushed a commit that referenced this issue Dec 9, 2017
The result of removing Style/SpaceInsideBrackets meant that the SpaceInside mixin was only getting utilized in Style/SpaceInsideParens. So this commit includes refactoring the mixin away and moving the necessary logic directly into Style/SpaceInsideParens. In addition, this commit renames the SurroundingSpace mixin's space_between? to space_after?, which is a more accurate description. See comment for more details.
@bbatsov bbatsov closed this as completed in 89445f9 Dec 9, 2017
@LandonSchropp
Copy link
Author

@bbatsov Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants