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

Do not generate unused components contents in `app:update` task #29645

Merged
merged 1 commit into from Jul 16, 2017

Conversation

@y-yagi
Copy link
Member

@y-yagi y-yagi commented Jul 1, 2017

Currently, app:update generates all contents regardless of the component using in application.

For example, even if not using Action Cable, app:update will generate a contents related to Action Cable. This is a little inconvenient.
This PR checks the existence of the component and does not generate unnecessary contents.
Can not check all options in this way. However, it will be able to prevent the generation of unnecessary files.

@rails-bot
Copy link

@rails-bot rails-bot commented Jul 1, 2017

r? @matthewd

(@rails-bot has picked a reviewer for you, use r? to override)

@kaspth
Copy link
Member

@kaspth kaspth commented Jul 2, 2017

Didn't we fix this with the .railsrc file?

@y-yagi
Copy link
Member Author

@y-yagi y-yagi commented Jul 2, 2017

Currently, app:update task does not use .railsrc.
Also, since the user who executed rails new and the user who executes app:update are not necessarily the same, .railsrc can not be used as it is.

@matthewd
Copy link
Member

@matthewd matthewd commented Jul 3, 2017

@kaspth we talked about it.. I think there might be an unmerged PR?

IIRC I wasn't excited about adding an extra top-level file for such a rare consideration (plus the potential confusion if you've added/removed a component since the initial generate), so this does seem potentially interesting to me.

@y-yagi
Copy link
Member Author

@y-yagi y-yagi commented Jul 3, 2017

Probably I think that it is about #22790

@y-yagi y-yagi force-pushed the y-yagi:check_component_when_run_app_update branch 2 times, most recently Jul 3, 2017
railties/lib/rails/app_updater.rb Outdated
def generator_options
options = { api: !!Rails.application.config.api_only, update: true }

unless defined?(ActiveRecord)

This comment has been minimized.

@rafaelfranca

rafaelfranca Jul 4, 2017
Member

Can we make the assumption that if ActiveRecord is defined we are loading Active Record? I don't think this is true since a gem can define this constant. Maybe we should check for the railties?

This comment has been minimized.

@y-yagi

y-yagi Jul 4, 2017
Author Member

It makes sense. I updated to check railties.

@y-yagi y-yagi force-pushed the y-yagi:check_component_when_run_app_update branch 4 times, most recently Jul 4, 2017
@kaspth
kaspth approved these changes Jul 15, 2017
Copy link
Member

@kaspth kaspth left a comment

I dig this! Simple, clean. Got 2 things, then 👍

railties/lib/rails/app_updater.rb Outdated
end

options
end

This comment has been minimized.

@kaspth

kaspth Jul 15, 2017
Member

Maybe we should condense these:

def generator_options
  options = { api: !!Rails.application.config.api_only, update: true }
  options[:skip_active_record] = !defined?(ActiveRecord::Railtie)
  options[:skip_action_mailer] = !defined?(ActionMailer::Railtie)
  options[:skip_action_cable]  = !defined?(ActionCable::Engine)
  options[:skip_sprockets]     = !defined?(Sprockets::Railtie)
  options[:skip_puma]          = !defined?(Puma)
  options
end

or even just make it a standard hash definition.

railties/CHANGELOG.md Outdated
@@ -1,3 +1,7 @@
* Do not generate unused components contents in `app:update` task.

This comment has been minimized.

@kaspth

kaspth Jul 15, 2017
Member

Let's flesh this out a bit more. How about:

*   Skip unused components when running `bin/rails app:update`

    If the initial app generation skipped Action Cable, Active Record etc.,
    the update task honors those skips too.
@kaspth
Copy link
Member

@kaspth kaspth commented Jul 15, 2017

Right, #22790 was what I was thinking of — and I was sure we had merged that. But this is far simpler 👍

@y-yagi y-yagi force-pushed the y-yagi:check_component_when_run_app_update branch Jul 15, 2017
Currently, `app:update` generates all contents regardless of the
component using in application.

For example, even if not using Action Cable, `app:update` will generate
a contents related to Action Cable. This is a little inconvenient.
This PR checks the existence of the component and does not generate
unnecessary contents.
Can not check all options in this way. However, it will be able to
prevent the generation of unnecessary files.
@y-yagi y-yagi force-pushed the y-yagi:check_component_when_run_app_update branch to 5803640 Jul 15, 2017
@y-yagi
Copy link
Member Author

@y-yagi y-yagi commented Jul 15, 2017

Thank you for review! I fixed.

@kaspth kaspth merged commit 1766e8e into rails:master Jul 16, 2017
1 of 2 checks passed
1 of 2 checks passed
continuous-integration/travis-ci/pr The Travis CI build failed
Details
codeclimate All good!
Details
@kaspth
Copy link
Member

@kaspth kaspth commented Jul 16, 2017

Nice!

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

Successfully merging this pull request may close these issues.

None yet

5 participants