Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use autoloading instead of requiring all files up front #1320

Merged
merged 1 commit into from
Aug 25, 2020

Conversation

vsppedro
Copy link
Collaborator

@vsppedro vsppedro commented Jul 13, 2020

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.

@vsppedro vsppedro force-pushed the replace-require-with-autoloading branch 2 times, most recently from 4f1b9ba to f861070 Compare July 15, 2020 14:28
@vsppedro vsppedro changed the title Replace require with autoload Use autoloading instead of requiring all files up front Jul 15, 2020
@vsppedro vsppedro force-pushed the replace-require-with-autoloading branch from 73624d2 to 2aa1c64 Compare July 16, 2020 02:13
@vsppedro vsppedro marked this pull request as ready for review July 16, 2020 02:14
@vsppedro vsppedro changed the title Use autoloading instead of requiring all files up front WIP - Use autoloading instead of requiring all files up front Jul 17, 2020
Copy link
Collaborator

@mcmire mcmire left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks pretty good so far! Just had a few comments so far on this.

@@ -26,3 +26,5 @@ def find_class!(name)
end
end
end

require 'shoulda/matchers/integrations/libraries'
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps this should remain in shoulda/matchers/integrations? If this is here then it won't get loaded until Registry is loaded.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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'
Copy link
Collaborator

@mcmire mcmire Jul 23, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
end

If 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.

Copy link
Collaborator Author

@vsppedro vsppedro Jul 25, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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!

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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'
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Never noticed that this constant isn't named the same as the file, whoops! 😅

Copy link
Collaborator Author

@vsppedro vsppedro Jul 25, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should I rename the class or the file? Or should I do nothing?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oops, thanks :)

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No problem!

@vsppedro vsppedro changed the title WIP - Use autoloading instead of requiring all files up front Use autoloading instead of requiring all files up front Jul 28, 2020
@mcmire
Copy link
Collaborator

mcmire commented Aug 24, 2020

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 vsppedro force-pushed the replace-require-with-autoloading branch from 3847893 to 50bfe11 Compare August 24, 2020 23:44
@vsppedro vsppedro force-pushed the replace-require-with-autoloading branch from 50bfe11 to 368c648 Compare August 24, 2020 23:46
@vsppedro
Copy link
Collaborator Author

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!

@mcmire mcmire merged commit ce78101 into thoughtbot:master Aug 25, 2020
@mcmire
Copy link
Collaborator

mcmire commented Aug 25, 2020

Awesome, thank you for your work on this!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants