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

Move the initializers rake task to Rails::Command #33631

Merged
merged 3 commits into from Aug 19, 2018

Conversation

Projects
None yet
7 participants
@anniecodes
Copy link
Contributor

anniecodes commented Aug 16, 2018

What this is doing:

  • Moves the initializers rake task to use Rails::Command without changing the API
  • Adds deprecation when calling it with bin/rake instead of bin/rails
  • Adds tests for the rake task since there wasn't any

cc/ @kaspth

anniecodes added some commits Aug 16, 2018

Update 'rake initializers' to use Rails::Command under the hood
* Invoke Rails::Command within the rake task
* Adds test for calling initializers with 'bin/rake'
* Adds deprecation warning to the rake task
@rails-bot

This comment has been minimized.

Copy link

rails-bot commented Aug 16, 2018

r? @eileencodes

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

desc "Print out all defined initializers in the order they are invoked by Rails."
task initializers: :environment do

This comment has been minimized.

@anniecodes

anniecodes Aug 16, 2018

Author Contributor

I got rid of loading the environment here since we are loading it within the Rails::Command. My assumption is that requiring it in both places would load it twice which is unnecessary, but please correct me if I'm wrong here .

The only down side of getting rid of it here is that our ActiveSupport::Deprecation.behavior will not be taken into consideration and therefore the warning is outputted to stderr instead of following whatever the user set up in their app. I thought it was pretty benign to have it output to stderr since this entire task will be removed in 6.1 but please let me know if you feel strongly about keeping the environment loading here.

@@ -1,8 +1,10 @@
# frozen_string_literal: true

require "rails/command"
require "active_support/deprecation"

desc "Print out all defined initializers in the order they are invoked by Rails."

This comment has been minimized.

@bogdanvlviv

bogdanvlviv Aug 16, 2018

Contributor

Could you please remove this desc? It would remove this deprecated rake task from the task list(rake -T)

This comment has been minimized.

@kaspth

kaspth Aug 19, 2018

Member

I know we've removed documentation before from other things, but I don't think that's a helpful habit or the best approach.

This comment has been minimized.

@matthewd

matthewd Aug 22, 2018

Member

@kaspth if it's deprecated, and the command is now documented under rails-proper, I don't see why we'd keep this?

This comment has been minimized.

@rafaelfranca

rafaelfranca Aug 22, 2018

Member

👍 to remove

This comment has been minimized.

@kaspth

kaspth Aug 22, 2018

Member

Ah yeah, that's right 👍

@kaspth kaspth merged commit cc14bf7 into rails:master Aug 19, 2018

2 checks passed

codeclimate All good!
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

bogdanvlviv added a commit to bogdanvlviv/rails that referenced this pull request Aug 22, 2018

@btyy77c btyy77c referenced this pull request Oct 8, 2018

Merged

Fix empty restrooms db during development #508

1 of 5 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.