Added ReflectionClassNameString cop

[Bhacaz]

This PR add a cop to check if the value of the option class_name of a ActiveRecord::Reflection is a string.

Why this cop:

I was working in a Rails project where often there was a constant used for class_name value. That caused a issue which was to preload unnecessary models use in the definition of a reflection.

---

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.

[mikegee]

This seems like a good cop to me. 👍

https://github.com/rubocop-hq/rubocop-rails also exists, so I think you'll want to add this cop to that repository too.

[koic]

FYI, since Rails 5.2 app if a string isn't used, the following error occurs.

/Users/koic/.rbenv/versions/2.6.0/lib/ruby/gems/2.6.0/gems/activerecord-5.2.2/lib/active_record/reflection.rb:436:in `initialize': A class was passed to `:class_name` but we are expecting a string. (ArgumentError)

rails/rails@e65aff7

Having auto-correct in the future will be useful when migrating from old Rails to new Rails.

[koic]

Looks good to me. Can you squash your commits into one?

[koic]

If you do not mind, could you please update the body of commit message with the description of the PR that you wrote using git commit --amend?

After merging this PR I'd like to forward porting it to the RuboCop Rails repository (Rails cops are extracted in the near future to this repository). Moving the repository will weaken the linkage with PR, so it will be helpful if details are written in the body of commit message.

[koic]

I merged this PR. And I will forward port it to the RuboCop Rails repository.

@mikegee Your comment is right, however I reviewed and merged this PR because it takes a little more time to adjust RuboCop Rails release 💦 Thank you as always.

[bquorning]

Using e.g. Account.name also returns a string, and protects against typos. I much prefer that style. Could the cop be made configurable?

[Bhacaz]

@bquorning Using Account.name will force load the class, instance of lazy loading it, which was the original reason to deprecate passing a class since Rails 5.2. rails/rails#27551

[ryym]

Shouldn't this cop allow a symbol value as well as a string?
class_name: :Account

[Bhacaz]

@ryym We can discuss about that (maybe in an issue), but using a symbol can quickly have its limits when using module.

Example: class_name: :Account::Admin will not work.

[cheerfulstoic]

I just came here to ask the same a @ryym

For your example you could use class_name: :'Account::Admin'.

The advice that I've generally heard is that symbols are better for memory usage than strings when you can use them

[Bhacaz]

@cheerfulstoic You're right that can works. But Reflection#class_name call .to_s on the value of class_name, so it will create a new String object unless it's already a string. I don't know what it's the best in term of memory but while using a symbol there will be one more object.

In the following example there will be 1 symbol and 2 strings (when using .to_s)

# ActiveRecord #1
has_one :account, class_name: :Account

# ActiveRecord #2
has_many :account, class_name: :Account

OR

2 strings

# ActiveRecord #1
has_one :account, class_name: 'Account'

# ActiveRecord #2
has_many :account, class_name: 'Account'

Proof that String#to_s return the same object.

options = { class_name: 'Account' }
p options[:class_name].object_id # => 70101446453420
@class_name = options[:class_name].to_s
p @class_name.object_id # => 70101446453420

options = { class_name: :Account }
p options[:class_name].object_id # => 6419108
@class_name2 = options[:class_name].to_s
p @class_name2.object_id # => 2347999460

Correct me if there something wrong

[ryym]

using a symbol can quickly have its limits when using module

Reflection#class_name call .to_s on the value of class_name

@Bhacaz I see. I didn't notice them. Thanks!

[cheerfulstoic]

I didn't know that it called to_s behind the scenes, but I suppose it makes sense. I guess I would have hoped that it went directly from a string/symbol to storing on the association object the class. But maybe it can't always do that

👍