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 COP Rails/DynamicFindAllBy which updates to where. #522

Closed
wants to merge 1 commit into from

Conversation

jasquat
Copy link

@jasquat jasquat commented Jul 23, 2021

This will add a new COP to update find_all_by_name to use where instead. This is basically a copy and paste from Rails/DynamicFindBy with updates for find_all_by.


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 (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.
  • 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.
  • If this is a new cop, consider making a corresponding update to the Rails Style Guide.

@koic
Copy link
Member

koic commented Jul 23, 2021

AFAIK, find_all_by_* has been removed from Rails 4.1 and RuboCop Rails also has conservative support for Rails 4 series (So, it doesn't add new features for the old Rails APIs).
#295 (comment)

Thank you for opening this PR. But this cop can be implemented as a third-party gem instead of RuboCop Rails.

@burnettk
Copy link

burnettk commented Jul 23, 2021

@koic do you know of any existing public rubocop gems that provide codemods for rails upgrades? that was really the intent of this cop (rails 3 to rails 4 in this case). would there be any chance the rubocop project itself would want to host that under rubocop/rails-upgrades or something? thanks for helping with the awesome rubocop project!

@koic
Copy link
Member

koic commented Jul 23, 2021

First, I think that Rails upgrades is one of role of RuboCop Rails. Moreover I would not want to maintain something like rubocop/rails-upgrades on rubocop org because I'd like to reduce the cost of maintaining old ones as much as possible.

This is a technical talk. Rails/DynamicFindBy cop was problematic with some false positives, such as find_by_id. I'm afraid that adding find_all_by_* will cause other new false positives, but for example find_all_by_id will have a low risk rather than find_by_id.

Conclusion, I could accept this feature if you will add find_by_all_* to the existing Rails/DynamicFindBy cop instead of new cop. I think that user impact and maintenance costs will be minimal.

NOTE: It was appropriate for this cop name to be named Rails/DynamicFinder, but the interface name cannot be changed with a minor version upgrade. Let's leave the name as Rails/DynamicFindBy.

@jasquat
Copy link
Author

jasquat commented Jul 26, 2021

@koic thanks. we were thinking about it again and realized that folks may not get a ton of use from this cop as written since it is entirely for the rails upgrade scenario. we were thinking that instead of pursuing this pull request and trying to get the change into the rubocop-rails suite that people will run continuously on their apps, we will keep working on the rails-upgrade related codemods in our branch and we may try to open source the overall upgrade scripts (maybe still pointing to the branch) in the future. that way, since the scripts are clearly intended to be used just once for each codebase, we won't have to be quite as formal about the tests and code and we can hopefully move a bit faster.

@jasquat jasquat closed this Jul 26, 2021
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.

3 participants