Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

fix warning: possibly useless use of a constant in void context #8424

Merged
merged 1 commit into from

5 participants

@kennyj
Collaborator

No description provided.

@carlosantoniodasilva

I noticed these warnings, but it's so ugly to fix them huh? Perhaps it'd be interesting to add a comment saying that it's specifically to avoid warnings.

/cc @fxn

@pixeltrix
Owner

Couldn't you create a dummy method and pass the constant as an argument - that'd kill the warning

@fxn
Owner
fxn commented

Oh, didn't notice, sorry for introducing them.

Yeah, :+1: on the edit with a comment, or silence warnings because we know in this case we want a constant evaled in void context.

@pixeltrix
Owner

Example:

def load_constant(constant)
  constant
end

load_constant A::B
@fxn
Owner
fxn commented

Too complicated for my taste.

Warnings are there to warn you in case you missed something. When you do know you are not missing the thing being warned the normal thing to do is to disable them in that piece of code.

@fxn
Owner
fxn commented

You know (untested):

silence_warnings do
  A::B
end

This is explicit and no comment is needed because the intention is clearly to silence warnings, that a posteriori one knows would be issued otherwise.

@pixeltrix
Owner

In this case I think silence_warnings is fine but generally I don't like wrapping it around code that may raise unexpected errors.

@kennyj
Collaborator

I like silence warnings in this case.
BTW, I find the following code

925     with_autoloading_fixtures do
926       ::A
927       ::A::B
928       ActiveSupport::Dependencies.remove_constant('A')
929       ActiveSupport::Dependencies.remove_constant('A::B')
930       assert !defined?(A)
931     end
932   end
933
934   def test_load_once_constants_should_not_be_unloaded
935     with_autoloading_fixtures do
936       ActiveSupport::Dependencies.autoload_once_paths = ActiveSupport::Dependencies.autoload_paths
937       ::A.to_s # ★
938       assert defined?(A)

Should we fix it ?

@fxn
Owner
fxn commented

Yeah, same principle for me.

@kennyj
Collaborator

I updated this PR.

@fxn fxn merged commit 396c068 into from
@pixeltrix
Owner

Here's something where silence_warnings could mask something:

>> class A; end
>> class B; end
>> class A::B; end
>> A::B
=> A::B
>> ActiveSupport::Dependencies.remove_constant('A::B')
=> A::B
(irb):3: warning: toplevel constant B referenced by A::B
=> B
>> silence_warnings{ A::B }
=> B

However, it's not going to affect this test case so it's fine :+1:

@carlosantoniodasilva

That was fast, thanks guys :)

@rafaelfranca

This line is still printing a warning.

/Users/rafaelmfranca/Projects/github/rails/activesupport/test/dependencies_test.rb:927: warning: possibly useless use of :: in void context

Any idea how to fix?

cc/ @fxn

Owner

How interesting.

Other similar warnings in this file are silenced by silence_warnings but not the one in line 927. Even more puzzling, if you add p 1 at the top of the file you'll see 1 printed after the warning. As if the warning was issued at parse time. Can't make sense of everything together. I'll investigate.

Owner

Mistery solved in 26c024e, Merry Christmas Rafael!

OMG! Merry Christmas to you too :heart:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
This page is out of date. Refresh to see the latest.
Showing with 7 additions and 3 deletions.
  1. +7 −3 activesupport/test/dependencies_test.rb
View
10 activesupport/test/dependencies_test.rb
@@ -923,8 +923,10 @@ def test_remove_constant_does_not_trigger_loading_autoloads
def test_remove_constant_does_not_autoload_already_removed_parents_as_a_side_effect
with_autoloading_fixtures do
- ::A
- ::A::B
+ silence_warnings do
+ ::A
+ ::A::B
+ end
ActiveSupport::Dependencies.remove_constant('A')
ActiveSupport::Dependencies.remove_constant('A::B')
assert !defined?(A)
@@ -934,7 +936,9 @@ def test_remove_constant_does_not_autoload_already_removed_parents_as_a_side_eff
def test_load_once_constants_should_not_be_unloaded
with_autoloading_fixtures do
ActiveSupport::Dependencies.autoload_once_paths = ActiveSupport::Dependencies.autoload_paths
- ::A.to_s
+ silence_warnings do
+ ::A
+ end
assert defined?(A)
ActiveSupport::Dependencies.clear
assert defined?(A)
Something went wrong with that request. Please try again.