Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with
or
.
Download ZIP

Loading…

replace use of MissingSourceFile with LoadError #8740

Merged
merged 1 commit into from

6 participants

@amatsuda
Collaborator

First of all, a top level constant, MissingSourceFile is just an alias of LoadError: https://github.com/rails/rails/blob/39374aa925a7d670b039c0c0c9aa9f4aef19466b/activesupport/lib/active_support/core_ext/load_error.rb#L25

Rails internally uses MissingSourceFile only in this particular case: https://github.com/rails/rails/blob/39374aa925a7d670b039c0c0c9aa9f4aef19466b/actionpack/lib/abstract_controller/helpers.rb#L172
and uses LoadError in all other places. I know the Rubyists say "diversity is good", but we don't need diversity like this.

Now the MissingSourceFile constant could be removed from ActiveSupport, but I don't think we're able to do it right now, because I found several libraries referencing a constant with this name, which might be expecting ActiveSupport to define it.

@carlosantoniodasilva

Should we at least raise a deprecation warning that the constant will be removed in future versions?

@nertzy

Probably should rename the method in the test too.

@senny
Owner

I think we should issue a deprecation warning when MissingSourceFile is used.

@jeremy
Owner

This is confusing, but the idea was to detect whether the AS autoloader couldn't find the file. That avoids masking an internal LoadError that the file may raise.

@rafaelfranca
Owner

We are not even raising this constant internally. We should remove its usage and deprecate it.

@rafaelfranca rafaelfranca merged commit 370bfda into rails:master
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Jan 4, 2013
  1. @amatsuda
This page is out of date. Refresh to see the latest.
View
2  actionpack/lib/abstract_controller/helpers.rb
@@ -169,7 +169,7 @@ def default_helper_module!
module_name = name.sub(/Controller$/, '')
module_path = module_name.underscore
helper module_path
- rescue MissingSourceFile => e
+ rescue LoadError => e
raise e unless e.is_missing? "helpers/#{module_path}_helper"
rescue NameError => e
raise e unless e.missing_name? "#{module_name}Helper"
View
17 activesupport/test/core_ext/load_error_test.rb
@@ -1,21 +1,6 @@
require 'abstract_unit'
require 'active_support/core_ext/load_error'
-class TestMissingSourceFile < ActiveSupport::TestCase
- def test_with_require
- assert_raise(MissingSourceFile) { require 'no_this_file_don\'t_exist' }
- end
- def test_with_load
- assert_raise(MissingSourceFile) { load 'nor_does_this_one' }
- end
- def test_path
- begin load 'nor/this/one.rb'
- rescue MissingSourceFile => e
- assert_equal 'nor/this/one.rb', e.path
- end
- end
-end
-
class TestLoadError < ActiveSupport::TestCase
def test_with_require
assert_raise(LoadError) { require 'no_this_file_don\'t_exist' }
@@ -29,4 +14,4 @@ def test_path
assert_equal 'nor/this/one.rb', e.path
end
end
-end
+end
View
4 activesupport/test/dependencies_test.rb
@@ -53,7 +53,7 @@ def test_tracking_identical_loaded_files
end
def test_missing_dependency_raises_missing_source_file
- assert_raise(MissingSourceFile) { require_dependency("missing_service") }
+ assert_raise(LoadError) { require_dependency("missing_service") }
end
def test_dependency_which_raises_exception_isnt_added_to_loaded_set
@@ -576,7 +576,7 @@ def test_constantize_shortcut_for_cached_constant_lookups
def test_nested_load_error_isnt_rescued
with_loading 'dependencies' do
- assert_raise(MissingSourceFile) do
+ assert_raise(LoadError) do
RequiresNonexistent1
end
end
Something went wrong with that request. Please try again.