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

[Style/EachWithObject] Add support for showing warning for positionally misplaced block parameters in reduce or each_with_object methods #8859

Closed
yedhink opened this issue Oct 7, 2020 · 3 comments · Fixed by #8916

Comments

@yedhink
Copy link

yedhink commented Oct 7, 2020

Platform: x86_64-linux
Shell: /bin/zsh
Ruby Version: 2.7.1
RuboCop Version: 0.92.0
Using Bundler: true

Is your feature request related to a problem? Please describe.

I'm a beginner in the Ruby world and was just trying out Rubocop. Thanks for creating this wonderful tool.
So when I was coding, I got a suggestion from rubocop to replace reduce with each_with_object. I learnt that there exists a method called each_with_object that way, which is super helpful. Thus I auto-formatted the code to follow each_with_object style and the error was fixed.

This is the new rubocop formatted code with each_with_object style:-

  def proj_substrings(
    base_string: "Howdy partner, sit down! How's it going?",
    dictionary: %w[below up down]
  )
    dictionary.each_with_object(Hash.new(0)) do |word, acc|
      is_substring = base_string.include? word.downcase
      acc[word] += 1 if is_substring
    end
  end
# output: {"down"=>1}

Then later after a few days, inorder to show others in my circle that Rubocop provides such an intelligent suggestion I tried to revert back to the reduce based code format, like so:-

 def proj_substrings(
    base_string: "Howdy partner, sit down! How's it going?",
    dictionary: %w[below up down]
  )
    dictionary.reduce(Hash.new(0)) do |word, acc|
      is_substring = base_string.include? word.downcase
      acc[word] += 1 if is_substring
      acc
    end
  end

So after reverting it back to above code, I couldn't get any suggestion to use each_with_object over reduce. I tried all stuffs including explicitly enabling the Style/EachWithObject: in .rubocop.yml. But still no errors were found.

Maybe it's my lack of experience, I only later saw the error in above code. When I reverted back to reduce based code, I didn't swap positions of the accumulator and word. In reduce method, accumulator should come as first block parameter and in each_with_object accumulator should come as second block parameter.
After making that modification by swapping positions of block params, I got the correct suggestion for Style/EachWithObject: for below code:-

 def proj_substrings(
    base_string: "Howdy partner, sit down! How's it going?",
    dictionary: %w[below up down]
  )
    dictionary.reduce(Hash.new(0)) do |acc, word|
      is_substring = base_string.include? word.downcase
      acc[word] += 1 if is_substring
      acc
    end
  end

Describe the solution you'd like

It would be super awesome, if rubocop has a rule to check whether the accumulator or the first block parameter is the one which is returned from reduce method block.
Example:- if I give:- dictionary.reduce(Hash.new(0)) {|acc, word| word}, then rubocop should atleast give a warning that the The accumulator "acc" is not the one which is returned.

Similarly if using each_with_object, like so:- dictionary.each_with_object(Hash.new(0)) { |acc, word| acc[word]+=1 }, it should give a warning or error which says The accumulator "word" is not the one being modified/accumulated

Describe alternatives you've considered

i'm not sure of the alternatives. I'm new to Ruby.

Additional context

You can paste the above mentioned code-blocks in a ruby file and just run rubocop on it to see for yourself, what I mean. If rubocop can show careless mistakes like the one I had done above, then it can save a lot of time for devs I believe.

Thanks

@dvandersluis
Copy link
Member

each_with_object doesn't need the accumulator to be returned:

[1,2,3,4].each_with_object([]) do |i, acc|
  acc.unshift(i)
  nil
end
#=> [4,3,2,1]

But this is a good point for reduce!

@yedhink
Copy link
Author

yedhink commented Oct 10, 2020

@dvandersluis Yes correct. With each_with_object we don't even need to return nil explicitly I think. The acc gets returned automatically I believe. Do correct me if I'm wrong. 😄

Also it would a really awesome feature to be implemented for reduce. I can definitely help with the PR. It's just that i'm new to ruby and not understanding a lot of portions in the codebase. If someone can help me with understanding the higher level overview of the codebase and how every component interacts, then definitely I can work on a fix on my own. 👍

@dvandersluis
Copy link
Member

dvandersluis commented Oct 10, 2020

@yedhink I was just returning nil in my example to prove that each_with_object doesn't care about the return value, whereas reduce sets the accumulator based on the return value.

I started working yesterday on a cop to catch some cases for reduce, hopefully I'll have a PR up soon!

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

Successfully merging a pull request may close this issue.

2 participants