Fix method definition cop #44

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-02-26T04:46:50Z

lib/rubocop/cop/rake/class_definition_in_task.rb

      # 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,


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,

@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.

lib/rubocop/cop/rake/class_definition_in_task.rb

lib/rubocop/cop/rake/helper/task_definition.rb

spec/rubocop/cop/rake/class_definition_in_task_spec.rb

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

pboling · 2025-04-05T00:51:28Z

lib/rubocop/cop/rake/class_definition_in_namespace.rb

      # 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 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,

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 💯