Undefine methods before redefining them #1584

sikachu · 2014-06-06T16:08:09Z

If you run RSpec test suite and turn on Ruby warnings (ruby -w), there are several warnings about method redefined:

/lib/rspec/core/configuration.rb:191: warning: method redefined; discarding old pattern=
/lib/rspec/core/example.rb:210: warning: method redefined; discarding old run
/lib/rspec/core/example.rb:204: warning: previous definition of run was here

By undef these method first we can silence those warnings from Ruby.

fables-tales · 2014-06-06T16:09:36Z

lib/rspec/core/configuration.rb

@@ -185,6 +185,7 @@ def output_stream=(value)
      # @macro add_setting
      # Load files matching this pattern (default: `'**/*_spec.rb'`)
      add_setting :pattern
+      undef :pattern=


Why is this one after the add_setting call. Does this one actually fix the warning?

It fixes it because there's a definition of pattern= below. I think there's a better approach, though: rather than using add_setting, use define_reader. add_setting is for cases where we want both the reader and writer methods generated but we want only the reader here.

Oh, right, I was confused a bit. Let me update this; just attr_reader make sense.

We actually need define_reader to be used -- our settings need that for the semantics of spec_helper settings being overridable when CLI options are used.

Oh, gotcha. I read that one wrong the first time.

fables-tales · 2014-06-06T16:10:32Z

@sikachu Can you make a little gist which highlights these warnings? It'd be good to see them in action. Also, it'd be nice if we could add some tests to this pull request to check that those warnings are actually no longer happening. Can you take a look at that?

myronmarston · 2014-06-06T16:44:51Z

Thanks @sikachu. We definitely want to be warning free. We already run our specs with --warnings but I'm realizing that takes effect after these files are loaded, which is why we haven't noticed these warnings.

What do you think about setting $VERBOSE = true at the top of script/rspec_with_simplecov? That should help us notice these sorts of things.

myronmarston · 2014-06-06T16:47:16Z

@samphippen -- here's the output you asked about:

✗ script/rspec_with_simplecov spec
/Users/myron/code/rspec-dev/repos/rspec-core/lib/rspec/core/configuration.rb:191: warning: method redefined; discarding old pattern=
/Users/myron/code/rspec-dev/repos/rspec-core/lib/rspec/core/example.rb:210: warning: method redefined; discarding old run
/Users/myron/code/rspec-dev/repos/rspec-core/lib/rspec/core/example.rb:204: warning: previous definition of run was here
Invalid gemspec in [/Users/myron/.gem/ruby/1.9.2/specifications/ZenTest-4.9.5.gemspec]: Illformed requirement ["< 3.0, >= 1.8"]
Run options:
  include {:focus=>true}
  exclude {:ruby=>#<Proc:./spec/spec_helper.rb:139>}

All examples were filtered out; ignoring {:focus=>true}
...............................................................................*............................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................

Pending:
  RSpec::Core::Metadata for an example points :example_group to the same hash object as other examples in the same group
    # Cannot maintain this and provide full `:example_group` backwards compatibility (see GH #1490):(
    # ./spec/rspec/core/metadata_spec.rb:111

Finished in 2.75 seconds (files took 0.97433 seconds to load)
1244 examples, 0 failures, 1 pending

Randomized with seed 9244

myronmarston · 2014-06-06T16:49:44Z

lib/rspec/core/example.rb

@@ -207,6 +207,7 @@ class Procsy
        Proc.public_instance_methods(false).each do |name|
          define_method(name) { |*a, &b| @proc.__send__(name, *a, &b) }
        end
+        undef :run


I think a better solution to this is to add a next if name.to_sym == :run in the Example.public_instance_methods(false).each do |name| loop. Then there's no reason to undef it.

sikachu · 2014-06-06T17:40:06Z

Yeah, I'll fix those issues and add $VERBOSE = true in that rspec_with_simplecov file. I'm not sure how or if I should add a test for this, though.

myronmarston · 2014-06-06T17:43:34Z

I'm not sure how or if I should add a test for this, though.

Don't worry about it.

If you run RSpec test suite and turn on Ruby warnings (`ruby -w`), there are several warnings about method redefined: * /lib/rspec/core/configuration.rb:191: warning: method redefined; discarding old pattern= * /lib/rspec/core/example.rb:210: warning: method redefined; discarding old run * /lib/rspec/core/example.rb:204: warning: previous definition of run was here By `undef` these method first we can silence those warnings from Ruby.

sikachu · 2014-06-06T18:06:58Z

@samphippen @myronmarston code is updated. How does it look now?

myronmarston · 2014-06-06T18:08:50Z

lib/rspec/core/example.rb

@@ -201,7 +201,9 @@ class Procsy
        attr_reader :example

        Example.public_instance_methods(false).each do |name|
-          define_method(name) { |*a, &b| @example.__send__(name, *a, &b) }
+          unless name.to_sym == :run


I tend to prefer using a next guard clause, but that's just subjective preference :). Not a merge blocker.

myronmarston · 2014-06-06T18:08:59Z

Looks good, I'll merge when green.

Undefine methods before redefining them

myronmarston · 2014-06-06T21:31:11Z

Thanks, @sikachu!

[ci skip]

Undefine methods before redefining them

[ci skip]

[ci skip] --- This commit was imported from rspec/rspec-core@4df38c6.

fables-tales reviewed Jun 6, 2014
View reviewed changes

myronmarston reviewed Jun 6, 2014
View reviewed changes

myronmarston added a commit that referenced this pull request Jun 6, 2014

Merge pull request #1584 from sikachu/remove-warnings

b6dd27a

Undefine methods before redefining them

myronmarston merged commit b6dd27a into rspec:master Jun 6, 2014

myronmarston added a commit that referenced this pull request Jun 6, 2014

Changelog for #1584.

4df38c6

[ci skip]

myronmarston added a commit that referenced this pull request Jun 6, 2014

Changelog for #1584.

1c2ef7a

[ci skip]

MatheusRich pushed a commit to MatheusRich/rspec-core that referenced this pull request Oct 30, 2020

Merge pull request rspec#1584 from sikachu/remove-warnings

ef6eed6

Undefine methods before redefining them

MatheusRich pushed a commit to MatheusRich/rspec-core that referenced this pull request Oct 30, 2020

Changelog for rspec#1584.

7e879d4

[ci skip]

yujinakayama pushed a commit to yujinakayama/rspec-monorepo that referenced this pull request Oct 6, 2021

[core] Changelog for rspec/rspec-core#1584.

771da2e

[ci skip] --- This commit was imported from rspec/rspec-core@4df38c6.

Uh oh!

Undefine methods before redefining them #1584

Undefine methods before redefining them #1584

Uh oh!

Conversation

sikachu commented Jun 6, 2014

Uh oh!

fables-tales Jun 6, 2014

Choose a reason for hiding this comment

Uh oh!

myronmarston Jun 6, 2014

Choose a reason for hiding this comment

Uh oh!

sikachu Jun 6, 2014

Choose a reason for hiding this comment

Uh oh!

myronmarston Jun 6, 2014

Choose a reason for hiding this comment

Uh oh!

sikachu Jun 6, 2014

Choose a reason for hiding this comment

Uh oh!

fables-tales commented Jun 6, 2014

Uh oh!

myronmarston commented Jun 6, 2014

Uh oh!

myronmarston commented Jun 6, 2014

Uh oh!

myronmarston Jun 6, 2014

Choose a reason for hiding this comment

Uh oh!

sikachu commented Jun 6, 2014

Uh oh!

myronmarston commented Jun 6, 2014

Uh oh!

sikachu commented Jun 6, 2014

Uh oh!

myronmarston Jun 6, 2014

Choose a reason for hiding this comment

Uh oh!

myronmarston commented Jun 6, 2014

Uh oh!

myronmarston commented Jun 6, 2014

Uh oh!

Uh oh!