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/RedundantBegin should detect redundant begin around memoization #9601

Closed
deivid-rodriguez opened this issue Mar 15, 2021 · 9 comments · Fixed by #9602
Closed

Style/RedundantBegin should detect redundant begin around memoization #9601

deivid-rodriguez opened this issue Mar 15, 2021 · 9 comments · Fixed by #9602

Comments

@deivid-rodriguez
Copy link
Contributor

@deivid-rodriguez deivid-rodriguez commented Mar 15, 2021

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

I'd like rubocop to be able to detect redundant begin/end blocks around memoization, like here: 3b48d8b.

Describe the solution you'd like

I think it would fit nicely inside the existing Style/RedundantBegin cop.

Describe alternatives you've considered

A separate cop if excluding this case was intentional, I guess.

@koic
Copy link
Member

@koic koic commented Mar 15, 2021

Good catch! I think it can enhance Style/RedundantBegin cop.

koic added a commit to koic/rubocop that referenced this issue Mar 15, 2021
…s around memoization

Fixes rubocop#9601.

This PR makes `Style/RedundantBegin` aware of redundant `begin`/`end` blocks
around memoization.
koic added a commit to koic/rubocop that referenced this issue Mar 16, 2021
…s around memoization

Fixes rubocop#9601.

This PR makes `Style/RedundantBegin` aware of redundant `begin`/`end` blocks
around memoization.
bbatsov pushed a commit that referenced this issue Mar 16, 2021
…d memoization

Fixes #9601.

This PR makes `Style/RedundantBegin` aware of redundant `begin`/`end` blocks
around memoization.
@deivid-rodriguez
Copy link
Contributor Author

@deivid-rodriguez deivid-rodriguez commented Mar 16, 2021

Thanks!!

@koic
Copy link
Member

@koic koic commented Mar 16, 2021

@deivid-rodriguez You are welcome! 😄

@tas50
Copy link
Contributor

@tas50 tas50 commented Mar 24, 2021

Thanks @deivid-rodriguez and @koic chef/chef#11220

@Drenmi
Copy link
Collaborator

@Drenmi Drenmi commented Apr 9, 2021

Would have been nice to have a configuration option, as we deliberately use this style for complex memoization expressions to avoid the indent-mega-blocks. 😬

@deivid-rodriguez
Copy link
Contributor Author

@deivid-rodriguez deivid-rodriguez commented Apr 9, 2021

@Drenmi Can you show an example of how this fix complicates indentation, if that's what you mean by indent-mega-blocks? Isn't

def foo
  @foo ||= begin
    mega_block do
      # ...      
      # a lot of code
      # ...
    end
  end
end 

equivalent to

def foo
  @foo ||=
    mega_block do
      # ...
      # a lot of code
      # ...
    end
end 

in terms of indentation? Or does it violate another common style rule?

@Drenmi
Copy link
Collaborator

@Drenmi Drenmi commented Apr 9, 2021

The auto-correction with our current settings leaves:

def something
  @something ||= mega_block do
                   # ...
                 end
end

after removing begin and applying other rules. Another small point that caused confusion/mistakes in the past is the mismatched end when not using begin.

@deivid-rodriguez
Copy link
Contributor Author

@deivid-rodriguez deivid-rodriguez commented Apr 9, 2021

Ah I see. Yeah, I've been there. I don't like this mega-indented blocks and I do recall having to change the code manually to the alternative I mentioned since rubocop auto-correct prefers the other style. I don't get the problem with mismatched end though, where is the mismatch?

@alexventuraio
Copy link

@alexventuraio alexventuraio commented Apr 22, 2022

Kind of the same issue from @Drenmi here:

# before
def decorator
  @decorator ||= begin
    case reminder.remindable_type
    when 'EmailSchedule'
      Notifier::EmailScheduleDecorator.new(reminder)
    when 'ReportScheduled'
      Notifier::ReportScheduledDecorator.new(reminder)
    else
      raise 'Unsupported type of Notifie'
    end
  end
end

# after
def decorator
  @decorator ||= case reminder.remindable_type
                  when 'EmailSchedule'
                    Notifier::EmailScheduleDecorator.new(reminder)
                  when 'ReportScheduled'
                    Notifier::ReportScheduledDecorator.new(reminder)
                  else
                    raise 'Unsupported type of Notifier'
                  end
end

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

Successfully merging a pull request may close this issue.

5 participants