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

Add new Style/MapCompactWithConditionalBlock cop #10608

Merged

Conversation

nobuyo
Copy link
Contributor

@nobuyo nobuyo commented May 7, 2022

This cop checks for usage of select or reject
instead of map { ... }.compact.

# bad
array.map { |e| some_condition? ? e : next }.compact

# bad
array.map do |e|
  if some_condition?
    e
  else
    next
  end
end.compact

# bad
array.map do |e|
  next if some_condition?

  e
end.compact

# bad
array.map do |e|
  e if some_condition?
end.compact

# good
array.select { |e| some_condition? }

# good
array.reject { |e| some_condition? }

Before submitting the PR make sure the following are checked:

  • The PR relates to only one subject with a clear title and description in grammatically correct, complete sentences.
  • Wrote good commit messages.
  • Commit message starts with [Fix #issue-number] (if the related issue exists).
  • Feature branch is up-to-date with master (if not - rebase it).
  • Squashed related commits together.
  • Added tests.
  • Ran bundle exec rake default. It executes all tests and runs RuboCop on its own code.
  • Added an entry (file) to the changelog folder named {change_type}_{change_description}.md if the new code introduces user-observable changes. See changelog entry format for details.

@nobuyo nobuyo force-pushed the add-new-style-redundant-map-compact-cop branch from d8bf005 to c768a4a Compare May 7, 2022 14:00
@nobuyo nobuyo marked this pull request as ready for review May 7, 2022 14:48
@nobuyo nobuyo marked this pull request as draft May 7, 2022 15:49
@nobuyo nobuyo force-pushed the add-new-style-redundant-map-compact-cop branch from c768a4a to c9daa11 Compare May 8, 2022 02:28
@nobuyo nobuyo marked this pull request as ready for review May 8, 2022 02:33
@nobuyo nobuyo force-pushed the add-new-style-redundant-map-compact-cop branch from c9daa11 to 55dc876 Compare May 8, 2022 23:03
@nobuyo nobuyo force-pushed the add-new-style-redundant-map-compact-cop branch from 55dc876 to d2dc8b3 Compare May 11, 2022 00:03
@nobuyo nobuyo changed the title Add new Style/RedundantMapCompact cop Add new Style/MapCompactWithConditionalBlock cop May 11, 2022
@nobuyo nobuyo changed the title Add new Style/MapCompactWithConditionalBlock cop Add new Style/MapCompactWithConditionalBlock cop May 11, 2022
@nobuyo
Copy link
Contributor Author

nobuyo commented May 11, 2022

Thank you for your review. I've renamed the cop and tweaked the message.

@nobuyo nobuyo requested review from koic and bbatsov May 12, 2022 00:30
@nobuyo nobuyo force-pushed the add-new-style-redundant-map-compact-cop branch from d2dc8b3 to bf4012e Compare May 12, 2022 03:47
config/default.yml Outdated Show resolved Hide resolved
Prefer `select` or `reject` over `map { ... }.compact`.

## example

```ruby
# bad
array.map { |e| some_condition? ? e : next }.compact

# bad
array.map do |e|
  if some_condition?
    e
  else
    next
  end
end.compact

# bad
array.map do |e|
  next if some_condition?

  e
end.compact

# bad
array.map do |e|
  e if some_condition?
end.compact

# good
array.select { |e| some_condition? }

# good
array.reject { |e| some_condition? }
```
@nobuyo nobuyo force-pushed the add-new-style-redundant-map-compact-cop branch from bf4012e to 0d55cb2 Compare May 13, 2022 12:20
@bbatsov
Copy link
Collaborator

bbatsov commented May 13, 2022

One more thing regarding the name - how about simply Style/MapCompact. I think the part about the block in the name is redundant and slightly confusing.

@nobuyo
Copy link
Contributor Author

nobuyo commented May 13, 2022

I'm fine with that.

@koic How do you think?

@bbatsov bbatsov requested a review from koic May 14, 2022 05:57
@koic
Copy link
Member

koic commented May 16, 2022

It may not be ambiguous to user because the department is different between Performance and Style, but I was concerned about the contrast with Performance/MapCompact cop. This cop seems to be more specific to a particular block than Performance/MapCompact cop.
https://docs.rubocop.org/rubocop-performance/1.13/cops_performance.html#performancemapcompact

That may be over-thinking, though...

@nobuyo
Copy link
Contributor Author

nobuyo commented May 18, 2022

So... Which should we name? I feel like they're both reasonable, but a little too geeky about the difference in Performance and Style.

@nobuyo nobuyo requested a review from bbatsov May 20, 2022 03:15
Copy link
Collaborator

@bbatsov bbatsov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Naming is hard. :-) Let's just go with the current name.

@bbatsov bbatsov merged commit 0e8d924 into rubocop:master May 26, 2022
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 this pull request may close these issues.

None yet

3 participants