Fix method definition cop by kuahyeow · Pull Request #44 · rubocop/rubocop-rake

kuahyeow · 2023-02-17T08:36:42Z

Fixes #42

Fixes ClassDefinitionInTask, and MethodDefinitionInTask to allow classes, modules and methods to defined inside tasks

kuahyeow · 2023-02-19T22:57:52Z

@koic Is the configured Circle CI still working. Am getting the error below:

 CircleCI Pipeline — Could not find a usable config.yml, you may have revoked the CircleCI OAuth app.

pboling

This is correct. The difference between which definition of a method or class wins is according to lexical order. The only one that works properly, without polluting the global Ruby Object namespace is inside a task, (whether or not it is inside a namespace is irrelevant).

pboling · 2023-11-09T09:52:33Z

Ping! Just ran into this false positive again, and came looking to see if any activity here.

pboling

LGTM! ❤️

q3aiml · 2023-12-22T17:54:55Z

Is this correctly handling when a task is nested inside a namespace? I believe that should be allowed, but the spec doesn't seem to cover it and it looks like it is disallowed.

For example this test fails:

  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

kuahyeow · 2024-01-23T03:56:35Z

That's true. I have now pushed a fix for this.

Is this correctly handling when a task is nested inside a namespace? I believe that should be allowed, but the spec doesn't seem to cover it and it looks like it is disallowed.

For example this test fails:
  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

pboling

LGTM

pboling · 2024-05-10T22:07:52Z

@kuahyeow The build passed before your last fix, but now it failing on Ruby >= 2.7. Any ideas?

kuahyeow · 2024-05-11T06:05:41Z

Looks like it's failing on unrelated rubocop errors....

rubocop
Inspecting 25 files
............C.......C....

Offenses:

lib/rubocop/cop/rake/helper/task_name.rb:11:30: C: [Correctable] InternalAffairs/NodeFirstOrLastArgument: Use #first_argument instead of #arguments[0].
            first_arg = node.arguments[0]
                             ^^^^^^^^^^^^
spec/rubocop/cop/rake/desc_spec.rb:4:6: C: [Correctable] InternalAffairs/ExampleDescription: Description does not match use of expect_offense.
  it 'register an offense for task on the top level' do
     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
spec/rubocop/cop/rake/desc_spec.rb:16:6: C: [Correctable] InternalAffairs/ExampleDescription: Description does not match use of expect_offense.
  it 'register an offense for task with block in a block' do
     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
spec/rubocop/cop/rake/desc_spec.rb:33:6: C: [Correctable] InternalAffairs/ExampleDescription: Description does not match use of expect_offense.
  it 'register an offense for task in kwbegin' do
     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

25 files inspected, 4 offenses detected, 4 offenses autocorrectable
rake aborted!
Command failed with status (1): [rubocop...]
/home/circleci/project/Rakefile:36:in `block in <top (required)>'
/home/circleci/.rubygems/gems/rake-12.3.3/exe/rake:27:in `<top (required)>'
/usr/local/bin/bundle:25:in `load'
/usr/local/bin/bundle:25:in `<main>'
Tasks: TOP => default => rubocop
(See full trace by running task with --trace)

Exited with code exit status 1

ydakuka · 2025-02-23T09:13:35Z

@kuahyeow could you please rebase the branch? The CI specs should pass as expected now.

pboling · 2025-02-23T13:24:35Z

LGTM still!

As methods defined in task do not pollute top level For example, this rakefile below will show that `do_something` does not appear in top level methods ``` task :a do def do_something puts 'a' end do_something end task :b do def do_something puts 'b' end do_something end puts methods.include?(:do_something) ```

As classes defined in task do not pollute top level. For example, this Rakefile shows class C in task does not pollute top level ``` task :a do class C def do_something puts 'a' end end C.new.do_something end task :b do class C def do_something puts 'b' end end C.new.do_something end puts Object.const_defined?('C') ```

Since the cop is now fixed to warn against class in namespaces, update the cop name accordingly.

Since the cop is now fixed to warn against method in namespaces, update the cop name accordingly.

This is similar to definitions within a task (without a namespace).

kuahyeow · 2025-03-10T05:24:53Z

@ydakuka @pboling Rebased now ! The rspecs workflow now requires approval from a maintainer to run.

pboling · 2025-04-05T00:46:43Z

@koic Please approve the workflow!

pboling

Needs a minor grammatical fix

kuahyeow · 2025-04-05T06:22:42Z

Thanks @pboling. I have updated the comment now. Please take a look !

…

On Sat, 5 Apr 2025 at 13:52, Peter Boling ***@***.***> wrote: ***@***.**** commented on this pull request. Needs a minor grammatical fix ------------------------------ In lib/rubocop/cop/rake/class_definition_in_namespace.rb <#44 (comment)>: > # 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 scope looks in the namespace, @kuahyeow <https://github.com/kuahyeow> This grammatical issue still needs to be fixed. It is incorrect to say it "the scope looks in the namespace", which may imply to some that the namespace is relevant to the scoping. It makes more sense to say it "appears as if it is in the namespace"... when in fact it is not. ⬇️ Suggested change - # It is confusing because the scope looks in the namespace, + # It is confusing because the scope appears as if it is in the namespace, — Reply to this email directly, view it on GitHub <#44 (review)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAAECU2FTZUXA4FCTBFQWFD2X4SLLAVCNFSM6AAAAABXWCUPDWVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZDONBUGM4DMMJWGM> . You are receiving this because you were mentioned.Message ID: ***@***.***>

pboling

LGTM 💯

tylerhunt · 2025-08-09T20:31:39Z

This one bit me today. Hoping for a merge and release soon.

rossme · 2025-08-17T16:46:22Z

@@ -30,12 +30,14 @@ module Rake
      #   task :foo do


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?

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

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!

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.

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.

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

pboling · 2025-08-18T08:40:31Z

This just bit me again, and I commented on this issue... checks notes, within the last day!
It can be hard to remember to do it the right way when I have to turn off this rule, and I very rarely write methods inside rake tasks. :(
♻️ refactoring my code now...

- rubocop/rubocop-rake#44