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

Change namespace of Active Storage rake task #30280

Merged

Conversation

koic
Copy link
Contributor

@koic koic commented Aug 16, 2017

Summary

The following is a display of the result of rake -T using the Active Storage application.

% bin/rake -T
rake about                              # List versions of all Rails frameworks and the environment
rake active_storage:install:migrations  # Copy migrations from active_storage to application
rake activestorage:install              # Copy over the migration needed to the application
rake app:template                       # Applies the template supplied by LOCATION=(/path/to/template) or URL

(snip)

The namespace of active_storage:install:migrations and activestorage:install are different. This PR unifies the namespace of Active Storage rake task to active_storage.

The namespace of active_storage:install:migrations depends on Engine#railtie_name.

namespace railtie_name do
namespace :install do
desc "Copy migrations from #{railtie_name} to application"
task :migrations do

So I think that the change target is namespace defined by activestorage.rake.

@rails-bot
Copy link

r? @matthewd

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

@kamipo kamipo merged commit 4fd5ceb into rails:master Aug 16, 2017
@koic koic deleted the change_namespace_of_active_storage_rake_task branch August 16, 2017 13:06
@matthewd
Copy link
Member

This feels to me like the sort of place we might deliberately choose the no-underscore spelling. They should be consistent... I just wonder which one we'd rather standardize on.

@dhh?

@dhh
Copy link
Member

dhh commented Aug 17, 2017

This problem should be going away shortly as the rake task is getting nixed in favor of a better way to install this migration. In general, I don't think these tasks should be prefixed by their framework. It's db:create, not active_record:db:create. And even if we did keep this rake task, it should be something like blob:migrations.

@sikachu
Copy link
Member

sikachu commented Aug 17, 2017

Yeah, I was thinking that ASt rake tasks could just be under storage prefix (such as, say, storage:clear_tmp). I think it's okay for activestorage:install to contain activestorage prefix though.

By the way, I think the correct fix might actually be making active_storage:install:migrations not showing up when do rake -T because it looks like we don't want user to call active_storage:install:migrations directly and call activestorage:install wrapper task instead.

@kamipo
Copy link
Member

kamipo commented Aug 17, 2017

Ah, I agree with active_storage:install:migrations should not be shown and active_storage is not preferable prefix.

@koic
Copy link
Contributor Author

koic commented Aug 17, 2017

Thank you for discussion.

I'd like to open a new PR with the following changes.

  • Rename active_storage:install task to blob:migrations task
  • Don't display active_storage:install:migrations task on rake -T

What do you think?

@koic
Copy link
Contributor Author

koic commented Aug 23, 2017

I opened a PR #30378 so that it would proceed on a code base.

@dhh
Copy link
Member

dhh commented Aug 23, 2017

@koic We're going to nix the need for an explicit rake task for this. At least for new apps. @sgrif is investigating what to do instead.

@koic
Copy link
Contributor Author

koic commented Aug 24, 2017

@dhh I got it. Thank you for sharing your information with me.

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

Successfully merging this pull request may close these issues.

None yet

6 participants