Use autoloading instead of requiring all files up front #1320
Conversation
vsppedro
force-pushed
the
replace-require-with-autoloading
branch
2 times, most recently
from
4f1b9ba
to
f861070
Compare
vsppedro
changed the title
vsppedro
force-pushed
the
replace-require-with-autoloading
branch
from
73624d2
to
2aa1c64
Compare
vsppedro
changed the title
| @@ -26,3 +26,5 @@ def find_class!(name) | |||
| end | |||
| end | |||
| end | |||
|
|
|||
| require 'shoulda/matchers/integrations/libraries' | |||
There was a problem hiding this comment.
Perhaps this should remain in shoulda/matchers/integrations? If this is here then it won't get loaded until Registry is loaded.
There was a problem hiding this comment.
No problem! I'll do that!
|
|
||
| module Shoulda | ||
| module Matchers | ||
| module Integrations | ||
| # @private | ||
| module TestFrameworks | ||
| autoload :ActiveSupportTestCase, 'shoulda/matchers/integrations/test_frameworks/active_support_test_case' |
There was a problem hiding this comment.
I think this is a case where these files can't be autoloaded, because there is a bit of magic here where upon being loaded, they will register themselves as possible test frameworks. This makes it possible to say e.g.
Shoulda::Matchers.configure do |config|
config.integrate do |with|
with.test_framework :rspec # or whatever
end
endIf the appropriate file that corresponds to the symbol you're setting isn't loaded then shoulda-matchers doesn't know what to do and the matcher methods will never be integrated into the appropriate test framework. Perhaps there is another way to accomplish this to where we can autoload them, but that's how they work now. This may be why the tests are failing.
There was a problem hiding this comment.
I made a silly mistake. I was only running unit tests.
I think the best strategy is to create a new branch of the master and make the changes little by little to see what went wrong. When finished I'll push to this PR.
This time I'll run all the tests at every change! 😅
Thanks for the info!
There was a problem hiding this comment.
Done! Tests fixed!
| @@ -176,6 +173,9 @@ def delegate_method(delegating_method) | |||
|
|
|||
| # @private | |||
| class DelegateMethodMatcher | |||
| autoload :StubbedTarget, 'shoulda/matchers/independent/delegate_method_matcher/stubbed_target' | |||
| autoload :DelegateObjectNotSpecified, 'shoulda/matchers/independent/delegate_method_matcher/target_not_defined_error' | |||
There was a problem hiding this comment.
Never noticed that this constant isn't named the same as the file, whoops! 😅
There was a problem hiding this comment.
Should I rename the class or the file? Or should I do nothing?
There was a problem hiding this comment.
I think this is fine as-is — some future update can fix this.
| @@ -1,14 +1,14 @@ | |||
| module Shoulda | |||
| module Matchers | |||
| module ActiveModel | |||
| module ActiveRecord | |||
vsppedro
changed the title
|
This looks good to me... does it look good to you? If so feel free to rebase/squash whenever you want to and I'll merge it in. |
vsppedro
force-pushed
the
replace-require-with-autoloading
branch
from
3847893
to
50bfe11
Compare
vsppedro
force-pushed
the
replace-require-with-autoloading
branch
from
50bfe11
to
368c648
Compare
|
Hi! It does look good to me! I was only waiting for a feedback on the constant that does not have the same name as the file. Conflict solved and rebase/squash done! |
|
Awesome, thank you for your work on this! |
Hi,
First, thanks for the opportunity to work on this issue: #1317
There still some files being loaded with require, I tried to use autoload but got some errors.
I'll keep digging for a solution.