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,

pboling · 2023-02-26T04:48:35Z

lib/rubocop/cop/rake/class_definition_in_task.rb

@@ -33,7 +33,7 @@ class ClassDefinitionInTask < Base

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


Actually this is the wrong test. It should be "in task". If not in task then it is using the Global Ruby Object namespace. It makes no difference if it is in a namespace or not.

Testing if in a namespace here may have been allowing you to avoid a test for the Global Object Space, but that's really what you want. According to this Cop's intention:

- Class, Module and Method definitions are OK iff defined at the top-level OR inside a task.

Class, Module, and Method definitions are not OK if defined inside a namespace without also being inside a task.

I don't think it can accurately capture that intent without a test for the definitions at the top level of the file.

@pboling Sorry, I dropped this completely. Must have slipped through the cracks

I don't think it can accurately capture that intent without a test for the definitions at the top level of the file.

There are such tests, which were pre-existing, please refer to:

"does not register an offense to def at the top level"

"allows class definition outside task"

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

Though I also see that the name of this cop is in-correct. Not sure how this is normally handled (is it breaking?) but I will push a rename for now.

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! ❤️

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.

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?

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

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