Skip to content

Fix method definition cop #44

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.

Sign up for GitHub

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

Open
kuahyeow wants to merge 7 commits into
base: master
Choose a base branch
from
Open

Fix method definition cop #44

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
eb38421
Fix MethodDefinitionInTask to allow methods defined in task
tkuah Feb 17, 2023
2b0dcde
Fix ClassDefinitionInTask to allow class defined in task
tkuah Feb 17, 2023
b82abd6
Remove un-used helper method
tkuah Feb 17, 2023
dcb54ac
Rename class definition cop to reflect fixed behaviour
tkuah Nov 11, 2023
973be1d
Rename method definition cop to reflect fixed behaviour
tkuah Nov 11, 2023
fc00ab1
Allow definitions within a task that is within a namespace.
tkuah Jan 23, 2024
7b9f68f
Clarify code comment
tkuah Apr 5, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 4 additions & 4 deletions config/default.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,8 +4,8 @@ Rake:
- 'Rakefile'
- '**/*.rake'

Rake/ClassDefinitionInTask:
Description: 'Do not define a class or module in rake task, because it will be defined to the top level.'
Rake/ClassDefinitionInNamespace:
Description: 'Do not define a class or module in a rake namespace, because it will be defined to the top level.'
Comment thread
kuahyeow marked this conversation as resolved.
Enabled: true
VersionAdded: '0.3.0'

Expand All @@ -24,7 +24,7 @@ Rake/DuplicateTask:
Enabled: true
VersionAdded: '0.4.0'

Rake/MethodDefinitionInTask:
Description: 'Do not define a method in rake task, because it will be defined to the top level.'
Rake/MethodDefinitionInNamespace:
Description: 'Do not define a method in a rake namespace, because it will be defined to the top level.'
Comment thread
kuahyeow marked this conversation as resolved.
Enabled: true
VersionAdded: '0.2.0'
14 changes: 8 additions & 6 deletions ...ocop/cop/rake/class_definition_in_task.rb → ...cop/rake/class_definition_in_namespace.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,13 +3,13 @@
module RuboCop
module Cop
module Rake
# Detects class or module definition in a task or namespace,
# Detects class or module definition in a namespace,
# because it is defined to the top level.
# It is confusing because the scope looks in the task or namespace,
# It is confusing because the class appears to be defined in the namespace,
# but actually it is defined to the top level.
#
# @example
# # bad
# # good
# task :foo do
# class C
# end
Expand All @@ -28,12 +28,14 @@ module Rake
# task :foo do
Comment thread
kuahyeow marked this conversation as resolved.
# end
#
class ClassDefinitionInTask < Base
MSG = 'Do not define a %<type>s in rake task, because it will be defined to the top level.'
class ClassDefinitionInNamespace < Base
MSG = 'Do not define a %<type>s in a rake namespace, because it will be defined to the top level.'

def on_class(node)
return if Helper::ClassDefinition.in_class_definition?(node)
return unless Helper::TaskDefinition.in_task_or_namespace?(node)
return if Helper::TaskDefinition.in_task?(node)

return unless Helper::TaskDefinition.in_namespace?(node)
Comment thread
kuahyeow marked this conversation as resolved.
Comment thread
kuahyeow marked this conversation as resolved.

add_offense(node, message: format(MSG, type: node.type))
end
Expand Down
22 changes: 18 additions & 4 deletions lib/rubocop/cop/rake/helper/task_definition.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,17 +8,31 @@ module TaskDefinition
extend NodePattern::Macros
extend self

def_node_matcher :task_or_namespace?, <<-PATTERN
def_node_matcher :namespace?, <<-PATTERN
Comment thread
kuahyeow marked this conversation as resolved.
(block
(send _ {:task :namespace} ...)
(send _ {:namespace} ...)
Comment thread
kuahyeow marked this conversation as resolved.
args
_
)
PATTERN

def in_task_or_namespace?(node)
def_node_matcher :task?, <<-PATTERN
(block
(send _ {:task} ...)
args
_
)
PATTERN

def in_namespace?(node)
Comment thread
kuahyeow marked this conversation as resolved.
node.each_ancestor(:block).any? do |a|
namespace?(a)
Comment thread
kuahyeow marked this conversation as resolved.
end
end

def in_task?(node)
node.each_ancestor(:block).any? do |a|
task_or_namespace?(a)
task?(a)
end
end
end
Expand Down
14 changes: 8 additions & 6 deletions ...cop/cop/rake/method_definition_in_task.rb → ...op/rake/method_definition_in_namespace.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,13 +3,13 @@
module RuboCop
module Cop
module Rake
# Detects method definition in a task or namespace,
# Detects method definition in a namespace,
# because it is defined to the top level.
# It is confusing because the scope looks in the task or namespace,
# It is confusing because the method appears to be defined in the namespace,
# but actually it is defined to the top level.
#
# @example
# # bad
# # good
# task :foo do
# def helper_method
# do_something
Expand All @@ -30,12 +30,14 @@ module Rake
# task :foo do

@rossme rossme Aug 17, 2025

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for this PR @kuahyeow 🙏

I don't think "expected behavior" is necessarily the same as a "good" example. It only moves the method definition but still leaves it at the top level of the file, which introduces the same problem of polluting globally. We've already shown a "good" example by adding it in the task directly.

Not sure what you think, but I would still find this misleading particularly if I was only running a single rake file, and there was a duplicate method definition in a later rake file.

I would remove this example, and add a cop for the top level method definition. What do you think?

@pboling pboling Aug 17, 2025

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree. Splitting the example so that only definitions inside the task are considered "good" would be good ;)

@pboling pboling Aug 17, 2025

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That would be aligned with my original report in #42, which I've copied here for easier reference

definition location scope rubocop-rake should consider rubocop-rake currently considers
top-level global bad 🛑 good ✅
namespace block global bad 🛑 bad 🛑
task block task good ✅ bad 🛑

"Good Code" example for task:

namespace :foo_namespace do
  desc 'foo task'
  task :foo do
    def greet
      puts 'Hello from foo.rake'
    end
 
    puts 'In foo task'
    greet
  end
end

"Bad Code" example for namespace:

namespace :foo_namespace do
  desc 'foo task'
  task :foo do
    puts 'In foo task'
    greet
  end
 
  def greet
    puts 'Hello from foo.rake'
  end
end

"Bad Code" example for top-level:

desc 'foo task'
task :foo do
  puts 'In foo task'
  greet
end
 
def greet
  puts 'Hello from foo.rake'
end

I think this should be the goal!

@kuahyeow kuahyeow Aug 17, 2025 •
edited
Loading

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @rossme for the comment. I think this is slightly related to #44 (comment), where I mentioned:

Given this cop's intention is to only highlight "surprising" behavior, I will leave this logic as is.

To keep this PR lean and tidy, my inclination is to make no further changes. Reason: Defining a top level method outside of any block is just like defining (and including) a top level method in any Ruby script. Reason 2: This example is pre-existing.

add a cop for the top level method definition

So, I would like this PR to be merged first (hopefully), and then we have another PR if there's need to this.

Hopefully a maintainer comes onboard at some point to review.

@rossme rossme Aug 17, 2025

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I do see your point about the top level method and keeping this PR lean 👍

I would still (gently) argue for adding a cop for any method outside of the task block seeing as we are only checking specific files ending in .rake and the specific quirks and features of Rake files.

Also removing the good example in a later PR if necessary also makes sense.

@corsonknowles corsonknowles Jul 2, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changing it from # good to # okay is the Rubocop doc way to distinguish what the cop accepts vs best practices

# end
#
class MethodDefinitionInTask < Base
MSG = 'Do not define a method in rake task, because it will be defined to the top level.'
class MethodDefinitionInNamespace < Base
MSG = 'Do not define a method in a rake namespace, because it will be defined to the top level.'

def on_def(node)
return if Helper::ClassDefinition.in_class_definition?(node)
return unless Helper::TaskDefinition.in_task_or_namespace?(node)
return if Helper::TaskDefinition.in_task?(node)

return unless Helper::TaskDefinition.in_namespace?(node)

add_offense(node)
end
Expand Down
4 changes: 2 additions & 2 deletions lib/rubocop/cop/rake_cops.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
# frozen_string_literal: true

require_relative 'rake/class_definition_in_task'
require_relative 'rake/class_definition_in_namespace'
require_relative 'rake/desc'
require_relative 'rake/duplicate_task'
require_relative 'rake/duplicate_namespace'
require_relative 'rake/method_definition_in_task'
require_relative 'rake/method_definition_in_namespace'
73 changes: 73 additions & 0 deletions spec/rubocop/cop/rake/class_definition_in_namespace_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,73 @@
# frozen_string_literal: true

RSpec.describe RuboCop::Cop::Rake::ClassDefinitionInNamespace, :config do
it 'allows class definition in task' do
expect_no_offenses(<<~RUBY)
task :foo do
class C
end
end
RUBY
end

it 'allows module definition in task' do
expect_no_offenses(<<~RUBY)
task :foo do
module M
end
end
RUBY
end

it 'allows class definition in a task when wrapped in a namespace' do
expect_no_offenses(<<~RUBY)
namespace :bar do
task :foo do
class C
end
end
end
RUBY
end

it 'registers an offense to a class definition in namespace' do
expect_offense(<<~RUBY)
namespace 'foo' do
class C
^^^^^^^ Do not define a class in a rake namespace, because it will be defined to the top level.
end
end
RUBY
end

it 'registers an offense to a module definition in namespace' do
expect_offense(<<~RUBY)
namespace 'foo' do
module M
^^^^^^^^ Do not define a module in a rake namespace, because it will be defined to the top level.
end
end
RUBY
end

it 'allows class definition in Class.new' do
expect_no_offenses(<<~RUBY)
task :foo do
Class.new do
class C
end
end
end
RUBY
end

it 'allows class definition at the top level' do
expect_no_offenses(<<~RUBY)
class C
end

task :foo do
end
RUBY
end
end
44 changes: 0 additions & 44 deletions spec/rubocop/cop/rake/class_definition_in_task_spec.rb
View file
Open in desktop

This file was deleted.

23 changes: 16 additions & 7 deletions ...op/rake/method_definition_in_task_spec.rb → ...ke/method_definition_in_namespace_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,11 +1,10 @@
# frozen_string_literal: true

RSpec.describe RuboCop::Cop::Rake::MethodDefinitionInTask, :config do
it 'registers an offense to `def` in a task' do
expect_offense(<<~RUBY)
RSpec.describe RuboCop::Cop::Rake::MethodDefinitionInNamespace, :config do
it 'does not register an offense to `def` in a task' do
expect_no_offenses(<<~RUBY)
task :foo do
def helper_method
^^^^^^^^^^^^^^^^^ Do not define a method in rake task, because it will be defined to the top level.
do_something
end
end
Expand All @@ -14,23 +13,33 @@ def helper_method
do_something

def helper_method
^^^^^^^^^^^^^^^^^ Do not define a method in rake task, because it will be defined to the top level.
do_something
end

def self.foo
^^^^^^^^^^^^ Do not define a method in rake task, because it will be defined to the top level.
do_something2
end
end
RUBY
end

it 'does not register an offense to `def` in a task when wrapped in a namespace' do
expect_no_offenses(<<~RUBY)
namespace :bar do
task :foo do
def helper_method
do_something
end
end
end
RUBY
end

it 'registers an offense to `def` in a namespace' do
expect_offense(<<~RUBY)
namespace 'foo' do
def helper_method
^^^^^^^^^^^^^^^^^ Do not define a method in rake task, because it will be defined to the top level.
^^^^^^^^^^^^^^^^^ Do not define a method in a rake namespace, because it will be defined to the top level.
do_something
end

Expand Down