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

Only load generators when needed #1374

Closed
wants to merge 3 commits into from

Conversation

dgynn
Copy link
Contributor

@dgynn dgynn commented Dec 13, 2015

This PR improves the way the generators are loaded and how they extend the resource generator.

  • The initializer block has been changed to a generator block which is only executed when generators are needed.
  • The call to app.load_generators has been removed. There is no need to load all generators.
  • The resource_override.rb has been changed to use hook_for to extend the resource generator.
  • The directory for the generators has been moved to match the way Rails looks to load generators.

With hook_for it would now be possible for a user to pass --no-serializer to skip that option.
The --serialize option also now shows up in the generator help with rails g resource --help.

These changes follow the way the Draper gem extends the controller generator.

@bf4
Copy link
Member

bf4 commented Dec 13, 2015

@dgynn thanks for this. Do you know if it these changes may solve the problem in #1335

Generator code is one I find hard to lookup. Did you find a good source, or just go source diving? If you know the rails plugins infrastructure well, I'd also appreciated your thoughts on #1352

Could you rebase this and add some more information to the generator section? I'd be ok with delaying that until #1371 is merged since it moves the docs around quite a bit. (Your review there is also appreciated :)

@dgynn
Copy link
Contributor Author

dgynn commented Dec 14, 2015

I've mostly been source diving to look into generators, initializers, and lazy hooks. It seems like a lot of people have copied code from similar examples. And possibly Rails 2 required doing extra stuff that is now unnecessary. I'll add some comments on #1352 but it does seem headed in the right direction.

I can add some docs and rebase after #1371 or see about doing a PR to your documentation branch.

For #1335, I think there was just confusion expecting that a config/initializer sample would have been generated. That doesn't seem necessary but I suppose one could be added if there are going to be more configuration options.

@bf4
Copy link
Member

bf4 commented Dec 14, 2015

see about doing a PR to your documentation branch.

Sounds great!

Anything you can share about your learnings, even in a gist or blog post or PR to rails guides would be much appreciated by me :)

@dgynn
Copy link
Contributor Author

dgynn commented Dec 21, 2015

I've submitted a rebased PR to your bf4/railties branch since there would be a conflict there. The documentation seems OK. And since the generator docs are in the Getting Started section it may confuse people to tell them how to skip the generator.

I've found another link that talks about this exact issue (extending the resource generator) and it even points back to this project. :) Here is what Jbuilder did to extend the scaffolding generator... rails/rails#9647

@dgynn dgynn closed this Dec 21, 2015
@dgynn dgynn deleted the pr_remove_load_generators branch January 23, 2016 19:18
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.

2 participants