-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
add require statements to top of file #1171
Conversation
@@ -1,14 +1,14 @@ | |||
require 'thread_safe' | |||
require_relative 'serializer/configuration' | |||
require_relative 'serializer/associations' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd rather use the full require active_model/serializer/configuration
without further discussion
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 to full require
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
np I'll update this.
EDIT: updated.
Awesome, thanks! |
Is there any sort of test you can add for this @shicholas, so your #1170 doesn't happen in the future? |
good question, I'm kind of stuck but I can't think of a test that won't introduce a ton of test dependencies (e.g. a |
perhaps we can get rid of all |
perhaps. I could get on board with that, even if there is a small performance and memory penalty in certain cases. |
Nice, link! I should note that Rubies 2.0+ are threadsafe with autoload ref: headius/thread_safe#15 (comment) Here's some links on require vs. require_relative performance in rspec |
I'm 👍 on getting rid of the autoload, but I'm okay with lazy loading |
I would ❤️ getting rid of the autoloads. Also |
I pushed an update to use all require statements. Thanks for the links, @bf4, I tried playing around w/ some autoload stuff to no avail (could be user error :)). |
Well, I'd say if we are going to do away with autoload, and since the issue was with autoload, and that coming up up with a proper test would be really weird / tricky / hacky, I think this in good to go as in. Good work! |
I'm having a tough time deciphering the build error messages; it looks like |
Our appveyer build breaks 80% of the time. We're working on it. Lol |
sorry, I realized that the test failures were my fault b/c I only removed the |
Note of caution, we're not actually using Kernel.autoload but ActiveSupport::Autoload, which cannot co-exist with require. Anyhow once we're using only requires, it won't matter. Also, requires should always be one direction, never circular |
👍 |
add require statements to top of file
Based on
#1170 (comment)