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

[Fix #51] Add ability to whitelist receiver class names #69

Merged
merged 1 commit into from
Apr 20, 2020

Conversation

tejasbubane
Copy link
Contributor

In Rails/DynamicFindBy.

Fixes #51.


Before submitting the PR make sure the following are checked:

  • 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.
  • Added an entry to the Changelog if the new code introduces user-observable changes. See changelog entry format.
  • The PR relates to only one subject with a clear title
    and description in grammatically correct, complete sentences.
  • Run bundle exec rake default. It executes all tests and RuboCop for itself, and generates the documentation.

@krzysiek1507
Copy link

Maybe we can check if the receiver inherits from ActiveRecord::Base?

@tejasbubane
Copy link
Contributor Author

Maybe we can check if the receiver inherits from ActiveRecord::Base?

This is not reliable. A class can extend from ApplicationRecord instead - or even from other classes.

@krzysiek1507
Copy link

A = Class.new ActiveRecord::Base 
B = Class.new A

B <= ActiveRecord::Base
=> true

@andyw8
Copy link
Contributor

andyw8 commented Jun 17, 2019

@krzysiek1507 RuboCop uses static analysis, It doesn't run the code, so this approach isn't possible.

@tejasbubane tejasbubane force-pushed the whitelist-receiver branch 2 times, most recently from 31cf95d to b5e9504 Compare June 17, 2019 18:21
@tejasbubane tejasbubane force-pushed the whitelist-receiver branch 3 times, most recently from c1894ce to 5e272f0 Compare July 2, 2019 17:47
@ngouy
Copy link
Contributor

ngouy commented Oct 17, 2019

Thank you for this PR :)

@tejasbubane
Copy link
Contributor Author

Rebased & fixed merge conflicts. @koic

@tejasbubane tejasbubane force-pushed the whitelist-receiver branch 3 times, most recently from 06a9074 to 0cfe340 Compare March 6, 2020 10:28
@tejasbubane tejasbubane force-pushed the whitelist-receiver branch 5 times, most recently from ddda677 to 5a9b9fe Compare April 19, 2020 12:11
@tejasbubane
Copy link
Contributor Author

@koic Fixed as per review comments.

@tejasbubane tejasbubane requested a review from koic April 19, 2020 12:13
In `Rails/DynamicFindBy`.

Also use `AllowedReceivers` & `AllowedMethods` instead of `Whitelist`
which will be deprecated soon.

Fixes rubocop#51.
@tejasbubane
Copy link
Contributor Author

@koic Thanks for the review :) I have fixed as per your suggestions.

@koic koic merged commit c36151c into rubocop:master Apr 20, 2020
@koic
Copy link
Member

koic commented Apr 20, 2020

Nicely done 👍

@tejasbubane tejasbubane deleted the whitelist-receiver branch April 20, 2020 15:42
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.

Rails/DynamicFindBy issue with Gem::Specification.find_by_name
5 participants