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

Use app namespace for framework tasks #23439

Merged
merged 1 commit into from Feb 27, 2016
Merged

Conversation

@ryohashimoto
Copy link
Contributor

@ryohashimoto ryohashimoto commented Feb 3, 2016

By introducing Rake proxy, rake rails commands become rails rails commands (rails rails:template, rails rails:update etc.) .

For simplicity, I removed the rails namespace from the framework codes and docs. (and now we can use rails template, rails update etc.)


updated:

Not removing the rails namespace, use app namespace instead.
e.g. rails rails:template and rails rails:update tasks should be rails app:template, rails app:update

@rails-bot
Copy link

@rails-bot rails-bot commented Feb 3, 2016

Thanks for the pull request, and welcome! The Rails team is excited to review your changes, and you should hear from @kaspth (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

@ryohashimoto ryohashimoto force-pushed the ryohashimoto:160203_rake_rails branch Feb 3, 2016
@@ -1,68 +1,66 @@
namespace :rails do

This comment has been minimized.

@ryohashimoto

ryohashimoto Feb 3, 2016
Author Contributor

In this file, only the namespace and indentations are changed.

@ryohashimoto ryohashimoto force-pushed the ryohashimoto:160203_rake_rails branch Feb 3, 2016
@kaspth
Copy link
Member

@kaspth kaspth commented Feb 3, 2016

I don't think we should change this right now. I think more of bin/rails as the runner, and that the commands it runs pertains to the app itself... mostly. Because some commands fiddle more with the framework, i.e. those in the rails namespace. I say we keep it like this for 5.0 and spare the deprecation cycle. Thanks!

@kaspth kaspth closed this Feb 3, 2016
@matthewd
Copy link
Member

@matthewd matthewd commented Feb 3, 2016

@kaspth should we change the namespace to app: or something? rails rails:foo does look pretty terrible.

@ryohashimoto
Copy link
Contributor Author

@ryohashimoto ryohashimoto commented Feb 3, 2016

@matthewd
I think so too.
The rails namespace is used for multiple purposes (update and template). So we could remove the namespace or use another namespace.

@kaspth
Copy link
Member

@kaspth kaspth commented Feb 3, 2016

Yeah, app sounds good.

We should still define the rails versions but enrich them with a deprecation prerequisite task.

@kaspth kaspth reopened this Feb 3, 2016
@ryohashimoto ryohashimoto force-pushed the ryohashimoto:160203_rake_rails branch 5 times, most recently Feb 4, 2016
@ryohashimoto
Copy link
Contributor Author

@ryohashimoto ryohashimoto commented Feb 4, 2016

@kaspth
Thanks for reopening this PR.
I updated the PR to use app namespace and add deprecation warnings for existing tasks under the rails namespace.

@ryohashimoto ryohashimoto changed the title Remove rails namespace for framework tasks Use app namespace for framework tasks Feb 4, 2016
@ryohashimoto ryohashimoto force-pushed the ryohashimoto:160203_rake_rails branch 2 times, most recently Feb 5, 2016
@kaspth
kaspth reviewed Feb 7, 2016
View changes
railties/CHANGELOG.md Outdated
@@ -1,3 +1,7 @@
* Use app namespace instead of rails namespace from the framework rake tasks.

This comment has been minimized.

@kaspth

kaspth Feb 7, 2016
Member

Needs more info. Mention some of the tasks and that the rails: namespace has been deprecated.

@kaspth
kaspth reviewed Feb 7, 2016
View changes
railties/lib/rails/tasks/framework.rake Outdated
task :deprecated do
Rake.application.top_level_tasks.grep(/^rails:/).each do |task|
$stderr.puts "WARNING: #{task} is deprecated. Please use #{task.sub(/^rails:/, 'app:')} instead."
end

This comment has been minimized.

@kaspth

kaspth Feb 7, 2016
Member

Don't need to print deprecation warnings for every task in the namespace. Likely more confusing than just printing the task they ran, if we can.

The message should mention that it's the namespace that is deprecated. Going with something like:

Running this task with the rails task namespace is deprecated in favor of app. Please run e.g. bin/rails app:update instead.

Could even remove the need to print the task altogether.

We should also require active_support and use ActiveSupport::Deprecation.warn, so people get their configured deprecation behavior to fire.

@kaspth
Copy link
Member

@kaspth kaspth commented Feb 7, 2016

Also needs a rebase (and remember to keep your commits squashed). Thanks ❤️

@ryohashimoto ryohashimoto force-pushed the ryohashimoto:160203_rake_rails branch 2 times, most recently Feb 13, 2016
@ryohashimoto
Copy link
Contributor Author

@ryohashimoto ryohashimoto commented Feb 13, 2016

@kaspth
Thanks for your advice.
Modified to use ActiveSupport::Deprecation.warn and removed deprecated task in the rails namespace.
So, when the task in the rails namespace is executed, only a deprecation message is printed out.

@kaspth
kaspth reviewed Feb 13, 2016
View changes
railties/lib/rails/tasks/framework.rake Outdated
Running this task with the rails task namespace is deprecated in favor of app.
Please run e.g. bin/rails #{task.sub(/^rails:/, 'app:')} instead."
MSG
end

This comment has been minimized.

@kaspth

kaspth Feb 13, 2016
Member

Sorry wasn't clear enough.

Don't need to print deprecation warnings for every task in the namespace.

I meant we shouldn't use Rake.application.top_level_tasks.grep(/^rails:/). I didn't mean to remove the deprecation enrichment of the rails: namespaced tasks, that part was correct before.

This comment has been minimized.

@kaspth

kaspth Feb 13, 2016
Member

We should replace this with:

task :deprecated do |task|
  ActiveSupport::Deprecation.warn(<<-MSG.squish)
    Running #{task.name.sub('deprecated:', '')} with the rails: namespace is deprecated in favor of app.
    Run e.g. bin/rails app:update instead."
  MSG
end

This comment has been minimized.

@ryohashimoto

ryohashimoto Feb 15, 2016
Author Contributor

I modified the codes for the :rails namespace in my local environment.

namespace :rails do
  task :deprecated do |task|
    ActiveSupport::Deprecation.warn(<<-MSG.squish)
      Running #{task.name.sub('deprecated:', '')} with the rails: namespace is deprecated in favor of app.
      Run e.g. bin/rails app:update instead."
    MSG
  end

  task :update => ['deprecated', 'app:update']
  task :template => ['deprecated',  'app:template']

  namespace :templates do
    task :copy => ['deprecated', 'app:templates:copy']
  end

  namespace :update do
    task :configs => ['deprecated', 'app:update:configs']
    task :bin => ['deprecated', 'app:update:bin']
  end
end

and ran the command.

$ rails rails:update                                                                  1 ↵  
DEPRECATION WARNING: Running rails:deprecated with the rails: namespace is deprecated in favor of app. Run e.g. bin/rails app:update instead.". (called from block in execute at /Users/ryo/.rbenv/versions/2.3.0/lib/ruby/gems/2.3.0/gems/rake-10.5.0/lib/rake/task.rb:238)

The message is incorrect because rails:deprecated isn't the original task used by the actual command.

This comment has been minimized.

@ryohashimoto

ryohashimoto Feb 15, 2016
Author Contributor

The code

Rake.application.top_level_tasks

returns only task names given in a command. (e.g. when rails rails:update is given, only [rails:update] is returned, not all tasks in the rails: namespace.)

So, I think the below snippet is correct.

namespace :rails do
  task :deprecated do |task|
    Rake.application.top_level_tasks.grep(/^rails:/).each do |task|
      ActiveSupport::Deprecation.warn(<<-MSG.squish)
        Running #{task} with the rails: namespace is deprecated in favor of app.
        Run e.g. bin/rails #{task.sub(/^rails:/, 'app:')} instead."
      MSG
    end
  end

  task :update => ['deprecated', 'app:update']
  task :template => ['deprecated',  'app:template']

  namespace :templates do
    task :copy => ['deprecated', 'app:templates:copy']
  end

  namespace :update do
    task :configs => ['deprecated', 'app:update:configs']
    task :bin => ['deprecated', 'app:update:bin']
  end
end

This comment has been minimized.

@ryohashimoto

ryohashimoto Feb 16, 2016
Author Contributor

OK, I understand.
I removed Rake.application.top_level_tasks.grep(/^rails:/)

@ryohashimoto ryohashimoto force-pushed the ryohashimoto:160203_rake_rails branch Feb 16, 2016
@ryohashimoto
Copy link
Contributor Author

@ryohashimoto ryohashimoto commented Feb 16, 2016

Updated and rebased. I think it's better to use deprecated task names like below.
(And it doesn't deprecate all tasks with rails: namespace.)

namespace :rails do
  %i(update template templates:copy update:configs update:bin).each do |task_name|
    task "#{task_name}" do
      ActiveSupport::Deprecation.warn(<<-MSG.squish)
        Running #{task_name} with the rails: namespace is deprecated in favor of app.
        Run e.g. bin/rails app:#{task_name} instead."
      MSG
      Rake.application.invoke_task("app:#{task_name}")
    end
  end
end
@ryohashimoto
Copy link
Contributor Author

@ryohashimoto ryohashimoto commented Feb 16, 2016

@kaspth
Could you take a look?

@ryohashimoto ryohashimoto force-pushed the ryohashimoto:160203_rake_rails branch Feb 16, 2016
@ryohashimoto ryohashimoto force-pushed the ryohashimoto:160203_rake_rails branch Feb 23, 2016
@kaspth
Copy link
Member

@kaspth kaspth commented Feb 24, 2016

Looks good and thanks for your patience! Can you rebase? If not, I will do it tonight 😄

@kaspth
Copy link
Member

@kaspth kaspth commented Feb 24, 2016

Wait, what do you mean by "(And it doesn't deprecate all tasks with rails: namespace.)"?

@ryohashimoto ryohashimoto force-pushed the ryohashimoto:160203_rake_rails branch Feb 24, 2016
@ryohashimoto
Copy link
Contributor Author

@ryohashimoto ryohashimoto commented Feb 24, 2016

@kaspth
Thanks for your support. Rebased now.

(And it doesn't deprecate all tasks with rails: namespace.)

I mean only tasks listed below are now deprecated. If someone add other tasks in the 'rails' namespace,
they shouldn't be deprecated because they don't have corresponding tasks in the 'app' namespace.
I believe this approach is natural.

%i(update template templates:copy update:configs update:bin) # in the rails namespace
@ryohashimoto ryohashimoto force-pushed the ryohashimoto:160203_rake_rails branch Feb 25, 2016
…amespace.

(e.g. `rails:update` and `rails:template` tasks is renamed to `app:update` and `app:template`.)
@ryohashimoto ryohashimoto force-pushed the ryohashimoto:160203_rake_rails branch to eaec958 Feb 26, 2016
kaspth added a commit that referenced this pull request Feb 27, 2016
Use app namespace for framework tasks
@kaspth kaspth merged commit 6fb3163 into rails:master Feb 27, 2016
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@kaspth
Copy link
Member

@kaspth kaspth commented Feb 27, 2016

@ryohashimoto thanks!

@ryohashimoto
Copy link
Contributor Author

@ryohashimoto ryohashimoto commented Feb 27, 2016

@kaspth Thanks a lot !

@ryohashimoto ryohashimoto deleted the ryohashimoto:160203_rake_rails branch Feb 27, 2016
kaspth referenced this pull request Feb 28, 2016
This guides were pointing to this command `rails app:update`, which I
tried to run, but it didnt worked. I think the right command is `rails
rails:update` instead. Also thats the name of the rake task.

Also I removed the `Rake` word from the title, as we run it using
`rails` bin now.

cc @kaspth
[skip ci]
yui-knk added a commit to yui-knk/rspec-rails that referenced this pull request Mar 23, 2017
Rake tasks `rails:update` or `rails:template` and so on were
deprecated on Rails 5.0, and will be removed on Rails 5.1.
So we should switch a task name.

ref:
* rails/rails#23439
* rails/rails@f778281
yui-knk added a commit to yui-knk/rspec-rails that referenced this pull request Mar 23, 2017
Rake tasks `rails:update` or `rails:template` and so on were
deprecated on Rails 5.0, and will be removed on Rails 5.1.
So we should switch a task name.

ref:
* rails/rails#23439
* rails/rails@f778281
yui-knk added a commit to yui-knk/rspec-rails that referenced this pull request Mar 23, 2017
Rake tasks `rails:update` or `rails:template` and so on were
deprecated on Rails 5.0, and will be removed on Rails 5.1.
So we should switch a task name.

ref:
* rails/rails#23439
* rails/rails@f778281
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
You can’t perform that action at this time.