-
Notifications
You must be signed in to change notification settings - Fork 280
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
Do not raise Utilityfunction for methods defined in a class_method block #1596
base: master
Are you sure you want to change the base?
Do not raise Utilityfunction for methods defined in a class_method block #1596
Conversation
lib/reek/context/method_context.rb
Outdated
result = @parent_exp.each_node(:send).select do |node| | ||
node_args = *node | ||
node_args.include?(:class_methods) | ||
end |
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.
Is there a way to pattern match
the s(:send, nil, class_methods)
node directly?
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.
You can check node.name == :class_methods
I am kind of hesitant to merge something that is specific to Rails but since it's seemingly used a lot...idk. Wdyt @mvz ? |
Yes, I'm also wondering how we can support this in a more generic way. Also, I think there's in issue that the proposed solution will suppress UtilityFunction in the whole module instead of just inside the |
@@ -177,6 +177,27 @@ def bravo(charlie) | |||
end | |||
end | |||
|
|||
context 'when examining class_methods block' do | |||
it 'reports on the refined class' do |
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.
This description seems wrong.
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.
Opps, ctrl + C problems
lib/reek/context/method_context.rb
Outdated
result = @parent_exp.each_node(:send).select do |node| | ||
node_args = *node | ||
node_args.include?(:class_methods) | ||
end |
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.
You can check node.name == :class_methods
|
||
expect(src).not_to reek_of(:UtilityFunction, context: 'Alpha#bravo') | ||
end | ||
end |
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.
Can you also add a test case that shows that methods defined outside the class_methods
block still trigger a UtilityFunction warning?
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.
Yup
To generalize this, we could add a configuration option to list which methods introduce a block that defines class methods. The default option would be empty, but users can set it to |
Excellent idea @mvz! |
@mvz nice idea, is there something in the project that I could reuse or base myself? Like a custom Rails configuration module or a class that does something similar |
@mvz seems like we forgot this one, I can see that you assigned this to yourself, any chance you pick it up again? |
Feel free to ping me again as well |
end | ||
RUBY | ||
|
||
expect(src).not_to reek_of(:UtilityFunction, context: 'Alpha#charlie') |
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.
@mateusluizfb Thanks for this PR!
I have a question about this line. charlie
is defined outside of the class_methods do
block.
So I would expect it to raise a UtilityFunction
warning.
Can you help me understand why this expectation is a .not_to reek_of
? Thanks.
Addresses #1538
This PR implements a way to not raise
UtilityFunction
warning for methods defined inclass_methods
block