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

Improve explanation for safety of MapCompact autocorrection in docs #450

Merged
merged 1 commit into from Apr 18, 2024

Conversation

Splines
Copy link
Contributor

@Splines Splines commented Apr 17, 2024

When I first read about the Performance/MapCompact cop, I didn't understand the following sentence at first glance:

This cop's autocorrection is unsafe because map { ... }.compact that is not compatible with filter_map.

  • The that in this sentence is confusing me grammatically-wise.
  • And this explanation did not hint me to why it's not safe. In hindsight, I don't know why I haven't just looked at the example one line above to see the actual difference. But maybe, for other users, it might be as well beneficial to improve the wording and refer to the given example to clearly point out that falsy values are filtered out by filter_map as well (see the Ruby docs). Compare that with compact, which only gets rid of nil values.

This fixes #280. The issue was already closed, however, it "fixes" it in the sense that the underlying confusion should be mitigated by a better wording in the docs.


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). -> ⚠ I've included the related issue it in the PR message as well as in the changelog entry. I forgot to add it to the commit message itself, hope that's ok.
  • Feature branch is up-to-date with master (if not - rebase it).
  • Squashed related commits together. -> ⚠ The first two commits of this PR could be squashed together. As I'm not rebasing very often, I'd have to look up how that works. If these two commits (that probably get squashed together into master anyways), do not impose a serious problem, maybe I can keep them as they are?
  • Added tests. -> ⚠ Not needed as this is just a small documentation change
  • 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.

🛑 As a sidenote: Note that in the last checklist item, you're linking to the contribution guide of rubocop (via the changelog entry format link), not that of this repo.

@Splines Splines marked this pull request as ready for review April 17, 2024 12:56
@koic
Copy link
Member

koic commented Apr 17, 2024

Once you have addressed the comments, please squash the commits into one.

@Splines
Copy link
Contributor Author

Splines commented Apr 17, 2024

Thanks for your review. I addressed all your points.

Note that I ran bundle exec rake default again and 8 examples failed in rspec. After that, I executed the command again and now everything is passing and RuboCop did not detect any offenses. After yet another execution of that command, still everything works as expected ;)

@koic koic merged commit 3e9d1bc into rubocop:master Apr 18, 2024
14 checks passed
@koic
Copy link
Member

koic commented Apr 18, 2024

Thanks!

@Splines Splines deleted the docs/mapcompact-safety branch April 18, 2024 16:36
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.

MapCompact cop is not correct because it does not give equivalent results
2 participants