-
-
Notifications
You must be signed in to change notification settings - Fork 263
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
ReflectionClassName: accept method calls on local variables #1180
base: master
Are you sure you want to change the base?
ReflectionClassName: accept method calls on local variables #1180
Conversation
12b998a
to
5932448
Compare
5932448
to
db9a8e9
Compare
@koic you're probably pretty busy, but I would love a review on this 😃 |
db9a8e9
to
a9eb6ae
Compare
@@ -124,6 +124,12 @@ | |||
RUBY | |||
end | |||
|
|||
it 'does not register an offense when parameter value is a method call on an object in a variable' do | |||
expect_no_offenses(<<~RUBY) | |||
has_many :accounts, class_name: the_class.name |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The ability to call the name
method suggests that, similar to has_many :accounts, class_name: Account.name
, an offense should be registered:
https://github.com/rubocop/rubocop-rails/pull/1180/files#diff-9d8842c039d0144f6548e139d525048bb9e0651ba63906ca875e35ff52140498R16-R25
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I understand the purpose of this cop to be to avoid the use of constant references, which lead to loading all the referenced classes.
Account.name
will resolve Account
, some_class_in_a_variable.name
will not load the class (it will already have been loaded).
I can see how this is kind of an edge case though since it does seem to require the loading of a class (unless the name
method here is not Class#name
).
I wonder - isn't your concern covered by it 'registers an offense when parameter value is a local variable assigned a constant'
?
I can look into this, change this test case to use different variable / method names and add another test case for
expect_offense(<<~RUBY)
the_class = Account
has_many :accounts, class_name: the_class.name
^^^^^^^^^^^^^^^^^^^^^^^^^^ Use a string value for `class_name`.
RUBY
(Although I think this makes me think that the message shouldn't be "Use a string value for class_name
", but rather "Don't use a constant reference for class_name
")
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, I tried it -
the_class = Account
has_many :accounts, class_name: the_class.name
does not register an offense even on main. So for now I've just changed the variable and method name in this test. I believe the behavior of the cop is as expected except for the above example which didn't work before either. But it's better to have a false negative than a false positive.
…flectionClassName
a9eb6ae
to
e4b0f53
Compare
@koic can we merge this? If not, what else is required? |
Fix #1179
Before submitting the PR make sure the following are checked:
[Fix #issue-number]
(if the related issue exists).master
(if not - rebase it).bundle exec rake default
. It executes all tests and runs RuboCop on its own code.{change_type}_{change_description}.md
if the new code introduces user-observable changes. See changelog entry format for details.If this is a new cop, consider making a corresponding update to the Rails Style Guide.