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

add require statements to top of file #1171

Merged
merged 5 commits into from
Sep 18, 2015
Merged

add require statements to top of file #1171

merged 5 commits into from
Sep 18, 2015

Conversation

shicholas
Copy link
Contributor

Based on
#1170 (comment)

@@ -1,14 +1,14 @@
require 'thread_safe'
require_relative 'serializer/configuration'
require_relative 'serializer/associations'
Copy link
Member

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

Copy link
Contributor

Choose a reason for hiding this comment

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

👍 to full require

Copy link
Contributor Author

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.

@bf4
Copy link
Member

bf4 commented Sep 17, 2015

Awesome, thanks!

@NullVoxPopuli
Copy link
Contributor

Is there any sort of test you can add for this @shicholas, so your #1170 doesn't happen in the future?

@shicholas
Copy link
Contributor Author

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 test/dummy rails app that is present in rails engines). Any thoughts? I'm happy to implement it.

@shicholas
Copy link
Contributor Author

perhaps we can get rid of all autoload like @bf4 mentioned here: http://www.benjaminfleischer.com/2013/07/18/ruby-requires-confusion/.

@NullVoxPopuli
Copy link
Contributor

perhaps. I could get on board with that, even if there is a small performance and memory penalty in certain cases.
@beauby, @joaomdmoura thoughts on that?

@bf4
Copy link
Member

bf4 commented Sep 17, 2015

@bf4
Copy link
Member

bf4 commented Sep 17, 2015

I'm 👍 on getting rid of the autoload, but I'm okay with lazy loading

@jfelchner
Copy link
Contributor

I would ❤️ getting rid of the autoloads. Also require_relative gives almost no performance improvement to require so I think we should just go with require since require_relative has its own problems.

@shicholas
Copy link
Contributor Author

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 :)).

@NullVoxPopuli
Copy link
Contributor

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!

@shicholas
Copy link
Contributor Author

I'm having a tough time deciphering the build error messages; it looks like simplecov?

@NullVoxPopuli
Copy link
Contributor

Our appveyer build breaks 80% of the time. We're working on it. Lol

@shicholas
Copy link
Contributor Author

sorry, I realized that the test failures were my fault b/c I only removed the autoload statements in lib/active_model/serializer.rb and not elsewhere. This is now updated and should now pass.

@bf4
Copy link
Member

bf4 commented Sep 18, 2015

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

@NullVoxPopuli
Copy link
Contributor

👍

NullVoxPopuli added a commit that referenced this pull request Sep 18, 2015
add require statements to top of file
@NullVoxPopuli NullVoxPopuli merged commit 24a5f38 into rails-api:master Sep 18, 2015
@shicholas shicholas deleted the require_statements branch September 18, 2015 14:39
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.

4 participants