Skip to content

Load models from eager_load, not autoload_paths#2771

Merged
mshibuya merged 2 commits into
railsadminteam:masterfrom
glebm:patch-1
Mar 20, 2017
Merged

Load models from eager_load, not autoload_paths#2771
mshibuya merged 2 commits into
railsadminteam:masterfrom
glebm:patch-1

Conversation

@glebm

@glebm glebm commented Nov 15, 2016

Copy link
Copy Markdown
Contributor

If something is in autoload_paths, it doesn't mean it can or should always be loaded. Rails explicitly makes this distinction with eager_load.

E.g. if I'm developing a Rails engine, I might want to add lib to autoload paths (to ease the development of the engine), but that doesn't mean I want to eagerly load ruby files in lib/generators/templates!

Resolves #2770

@impurist

Copy link
Copy Markdown

This makes sense to me. I currently have to explicitly exclude classes in lib. Annoying!
Your idea is valid.However!
eager_load is about table names in relational queries with ActiveRecord and not about loading model classes with Ruby. These are 2 very different little beasts.

@glebm

glebm commented Nov 15, 2016

Copy link
Copy Markdown
Contributor Author

@impurist ActiveRecord eager_load and config.eager_load_paths are unrelated.

@glebm

glebm commented Nov 29, 2016

Copy link
Copy Markdown
Contributor Author

@mshibuya どう思いますか?

@impurist

Copy link
Copy Markdown

So eager_load_paths is poorly named

@mshibuya

Copy link
Copy Markdown
Member

Agreed, but it would be better to have a spec that will fail without this change.

If something is in `autoload_paths`, it doesn't mean it can or should *always* be loaded. Rails explicitly makes this distinction with `eager_load`.

E.g. if I'm developing a Rails engine, I might want to add `lib` to autoload paths (to ease the development of the engine), but that doesn't mean I want to eagerly load ruby files in `lib/generators/templates`!

Resolves railsadminteam#2770
@glebm

glebm commented Mar 18, 2017

Copy link
Copy Markdown
Contributor Author

@mshibuya Done

@mshibuya

Copy link
Copy Markdown
Member

Nice, can be merged if the rubocop offense is fixed.

@glebm

glebm commented Mar 20, 2017

Copy link
Copy Markdown
Contributor Author

@mshibuya Sorry, didn't notice it. Now fixed!

@mshibuya mshibuya merged commit ead1775 into railsadminteam:master Mar 20, 2017
@mshibuya

Copy link
Copy Markdown
Member

Thanks for the fix!

@glebm glebm deleted the patch-1 branch June 7, 2018 18:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants