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

Fixed typo for AssertIncludes cop. Changed includes -> include #19

Merged
merged 1 commit into from Oct 2, 2019

Conversation

abhaynikam
Copy link
Contributor

This PR fixes the typo for AssertIncludes cop. Changes collection.includes -> collection.include at all places.

@koic
Copy link
Member

koic commented Oct 1, 2019

The change fixes a false negative. Can you please add the changelog entry?

@abhaynikam
Copy link
Contributor Author

@koic : Added the changelog. Can you please have a look?

CHANGELOG.md Outdated
@@ -2,6 +2,10 @@

## master (unreleased)

### Bug fixes

* [#19](https://github.com/rubocop-hq/rubocop-minitest/pull/19): Fixes typo `collection.includes?` to `collection.include?` in `Minitest/AssertIncludes` cop. ([@abhaynikam][])
Copy link
Member

Choose a reason for hiding this comment

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

It would be better to write a false negative correction rather than a typo :-) The following is an example.

Fix a false negative for `Minitest/AssertIncludes` when using `include` method in arguments of `assert` method.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, @koic 🎉 . Updated the changelog

@bbatsov
Copy link
Contributor

bbatsov commented Oct 1, 2019

Btw, I wonder if we should check for both for the sake of Rails users. I know the ActionSupport aliases are quite popular there. @koic any thoughts?

@koic
Copy link
Member

koic commented Oct 1, 2019

It seems my understanding is lacking 💦 Which Active Support API is that?

@bbatsov
Copy link
Contributor

bbatsov commented Oct 1, 2019

Hmm, that's weird - I can't find it here https://edgeguides.rubyonrails.org/active_support_core_extensions.html#extensions-to-enumerable I could swear that back in the day Rails shipped some aliases like includes?, contains?, etc. Perhaps they were removed at some point?

@koic
Copy link
Member

koic commented Oct 1, 2019

@bbatsov Thanks for the answer. AFAIK, there is an in? method that works like include? method.
https://guides.rubyonrails.org/active_support_core_extensions.html#in-questionmark

Other methods cannot be remembered 😅 First of all, I think that it is better to support only include? method respond by Minitest.
https://github.com/seattlerb/minitest/blob/930ec0ba2e3ca010cca388a0429b33fd63c7d0bd/lib/minitest/assertions.rb#L239-L248

@bbatsov
Copy link
Contributor

bbatsov commented Oct 2, 2019

Yeah, I agree.

@bbatsov bbatsov merged commit 0a06871 into rubocop:master Oct 2, 2019
@abhaynikam abhaynikam deleted the fix-typo-for-assert-includes branch October 2, 2019 12:25
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