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

SpaceInsideHashLiteralBraces fails on nested hashes #2595

Closed
brewster1134 opened this issue Jan 7, 2016 · 11 comments
Closed

SpaceInsideHashLiteralBraces fails on nested hashes #2595

brewster1134 opened this issue Jan 7, 2016 · 11 comments

Comments

@brewster1134
Copy link

i noticed that a hash like h = { a: 1, { b: 2 }} will fail since it expects h = { a: 1, { b: 2 } }
is this desired/standard style? i have always written it as the former. if the former should be allowed, i could take a stab at writing a spec for it (i am not familiar with this codebase however ;)

@alexdowad
Copy link
Contributor

So you are suggesting that there should be space inside a hash literal... unless the last value (or first key) is itself a hash?

@brewster1134
Copy link
Author

exactly. i adopted that style after seeing it used in really popular codebases (rails & mongoid off the top of my head). e.g. the following hashes would be valid

{{ a: 1 }, b: 2 }
{ a: 1, { b: 2 }}
{ a: 1, { b: 2 }, c: 3 }

@jawshooah
Copy link
Contributor

Guessing that the second should be

{ a: 1, { b: 2 }}

@brewster1134
Copy link
Author

@jawshooah yup, updated it! thnx!

@brewster1134
Copy link
Author

i have also found instances from rspec where there is a hash argument in a Proc

# meh
subject { Foo.new({ foo: 'bar' }, type: :hash) }

# looks better (to me)
subject{ Foo.new({ foo: 'bar' }, type: :hash )}

lots of edge cases with this cop it seems...

@jonas054
Copy link
Collaborator

subject{ Foo.new({ foo: 'bar' }, type: :hash )}

Examples don't give the whole picture. It better if you can express all rules that would define such a style. Something like:

  • The opening block brace should be preceded by a space, except in the following conditions:
    • ...
  • There should be space inside bock braces except for:
    • ...
  • There should be space inside hash literal braces except for:
    • ...
  • There should be no space inside method call parentheses except for:
    • ...

There's a lot to be said for simple rules. It would be interesting to see these rules listed, but I'm not too keen on making things so complicated.

@alexdowad
Copy link
Contributor

@brewster1134 What do you think about cases where a hash literal appears inside a block, like this:

method { |h| h.merge { key: val }}

Do you prefer to see a space between the concluding braces, or not?

@brewster1134
Copy link
Author

@alexdowad the convention i have seen, would be just like how you have it. i think basically the rule is you always want to avoid ) }, ] }, } }

@jonas054 when i have some time, i will sit down and define some specific rules (unless the examples above cover everything ha!). I just need to sit down and think it out... soon.... 😄

@brewster1134
Copy link
Author

@alexdowad so after sitting down and writing out a variety of use cases, i think it should actually only apply to braces.

  • when 2 or more opening braces are separated with a space(s), the spaces should be removed, and a single space should follow the last brace: { {{ { -> {{{{
  • when 2 or more closing braces are separated with a space(s), the spaces should be removed, and a single space should proceed the first brace: } }} } -> }}}}

@alexdowad
Copy link
Contributor

Can you suggest a name for this new style? We already have space and no_space.

@brewster1134
Copy link
Author

smart_spacing? compact_spacing? umm weighted_spacing?
im not great with naming 😄 it seems like name should also highlight the fact the space is on the left or right depending on if its an opening/closing brace (not sure if that was clear from the bulleted examples above)

@bbatsov bbatsov closed this as completed in 142d059 Jun 6, 2016
Neodelf pushed a commit to Neodelf/rubocop that referenced this issue Oct 15, 2016
...As requested by Brewster. I personally also prefer this style on my own
projects.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants