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

Discuss and setting up Style/HashAsLastArrayItem cop #199

Closed
roman-dubrovsky opened this issue Jul 17, 2020 · 7 comments
Closed

Discuss and setting up Style/HashAsLastArrayItem cop #199

roman-dubrovsky opened this issue Jul 17, 2020 · 7 comments
Assignees
Labels
fixed in new library version the issue has been reported and fixed in library repository but has not been merged or released yet help wanted Extra attention is needed high priority Critical thing which should be added asap question Further information is requested
Milestone

Comments

@roman-dubrovsky
Copy link
Contributor

Is your feature request related to a problem? Please describe.
Here I just want to discuss rule for this cop.

This cop sets up a rule for hash braces in the array.

EnforcedStyle: braces (default)

# bad
[1, 2, one: 1, two: 2]

# good
[1, 2, { one: 1, two: 2 }]

EnforcedStyle: no_braces (our rule)

# bad
[1, 2, { one: 1, two: 2 }]

# good
[1, 2, one: 1, two: 2]

cons of each rule on real examples:

For braces rule

In the include blocks we need to add hash braces

Post.unscoped.includes({account_holders: [{user: :profile}]})

Usually, I write something like this

Post.unscoped.includes(account_holders: [user: :profile])

For no_braces rule, usually, I faced the issue on creating an expected response for any API

Instead of writing some array which contains list of object

let(:contacts_list) do
  {
    contacts: [{contactID: 'test'}]
  }
end

I need to write this variant

let(:contacts_list) do
  {
    contacts: [contactID: 'test']
  }
end

But if says a truth it can be really easy refactored

let(:contact) { {contactID: 'test'}  }
let(:contacts_list) { {contacts: [contact] } }

What do you think about it?

@roman-dubrovsky roman-dubrovsky added help wanted Extra attention is needed question Further information is requested high priority Critical thing which should be added asap labels Jul 17, 2020
@roman-dubrovsky roman-dubrovsky self-assigned this Jul 17, 2020
@roman-dubrovsky roman-dubrovsky added this to the 0.10.1 milestone Jul 17, 2020
@roman-dubrovsky
Copy link
Contributor Author

roman-dubrovsky commented Jul 17, 2020

P.S. Also right now no_braces has a bug for array of hashes. It converts this expression

contacts = [{contactID: 'test'}, {contactID: 'test2'}]

to this one

contacts = [contactID: 'test', contactID: 'test2']

which is equal [{:contactID=>"test2"}]

But I'm going to report this issue to rubocop library

P.P.S. Seems like this issue has been fixed rubocop/rubocop#8323

@roman-dubrovsky roman-dubrovsky changed the title Title goes here Discuss and setting up Style/HashAsLastArrayItem cop Jul 17, 2020
@roman-dubrovsky
Copy link
Contributor Author

Personally, I still love no_braces rule more

@texpert
Copy link
Contributor

texpert commented Jul 19, 2020

I'm also for no_braces.

@DDKatch
Copy link
Contributor

DDKatch commented Jul 20, 2020

[1, 2, one: 1, two: 2] may mislead to think that the real value here is [1, 2, {one: 1}, {two: 2}] but actually [1, 2, {one: 1, two: 2}]. It may lead to mistakes.

@DDKatch
Copy link
Contributor

DDKatch commented Jul 20, 2020

[contactID: 'test'] convertes to [{:contactID=>"test2"}]. I always thought it's how ruby works 🤔
The issue(rubocop/rubocop#8323) was about parsing errors for a multiline array of hashes

@roman-dubrovsky
Copy link
Contributor Author

roman-dubrovsky commented Jul 21, 2020

[1, 2, one: 1, two: 2] may mislead to think that the real value here is [1, 2, {one: 1}, {two: 2}] but actually [1, 2, {one: 1, two: 2}]. It may lead to mistakes.

@DDKatch are you sure? in you case, you should write something like this (by this cop) [1, 2, {one: 1}, two: 2]. It looks suck but it's easy for refactoring or we can patch this rubocop rule in the future (e.g. restrict this rule for multiple array)

[contactID: 'test'] convertes to [{:contactID=>"test2"}]. I always thought it's how ruby works 🤔
The issue(rubocop/rubocop#8323) was about parsing errors for a multiline array of hashes

in my example above, ruby just overrides the value for contactID key . Or I didn't understand your issue 🤔

@roman-dubrovsky roman-dubrovsky modified the milestones: 0.10.1, 0.11.0 Aug 2, 2020
@roman-dubrovsky roman-dubrovsky added the fixed in new library version the issue has been reported and fixed in library repository but has not been merged or released yet label Aug 2, 2020
@roman-dubrovsky roman-dubrovsky modified the milestones: 0.11.0, 1.0.0 Nov 7, 2020
@roman-dubrovsky
Copy link
Contributor Author

this issue has been fixed #199 (comment)

from documentation:

# good
[{ one: 1 }, { two: 2 }]

So I'm going to close this issue and leave no_braces style

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
fixed in new library version the issue has been reported and fixed in library repository but has not been merged or released yet help wanted Extra attention is needed high priority Critical thing which should be added asap question Further information is requested
Projects
None yet
Development

No branches or pull requests

3 participants