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

Add ActionText installer rake task #37823

Merged

Conversation

abhaynikam
Copy link
Contributor

@abhaynikam abhaynikam commented Nov 28, 2019

After changes in #35085

While I was debugging this #37818, I faced with the issue where rails action_text:install as mentioned in the documentation was not working. After debugging for long it landed me to this PR: #35085

PR moved the ActionText installer from task to generator. The documentation and warning message where user was suggested to run generator was not updated. This PR update the mention on ActionText installer.

I am not confident about changes in tasks/release file. Please do let me know if we need to revert it.

@abhaynikam
Copy link
Contributor Author

abhaynikam commented Dec 4, 2019

@kaspth
Copy link
Contributor

kaspth commented Dec 15, 2019

This ain't right. We should have an action_text:install command that forwards to generate action_text:install. cc @vinistock

@vinistock
Copy link
Contributor

vinistock commented Dec 15, 2019

@kaspth maybe I misunderstood, but I thought the intention was to get rid of the rake task.

We can add it back in if that's the desire. Just re-create actiontext/lib/tasks/actiontext.rake and make it invoke the generator. @abhaynikam would you like to tackle it? If not I can put together a PR.

@abhaynikam
Copy link
Contributor Author

abhaynikam commented Dec 15, 2019

Thanks for the feedback @kaspth. @vinistock I would love to.

@abhaynikam abhaynikam force-pushed the 35085-update-the-action-text-installer branch from 00e5b3d to fb6248c Compare Dec 15, 2019
@abhaynikam abhaynikam changed the title Updated the ActionText installer command in the warning message and documentation Add ActionText installer rake task Dec 15, 2019
@abhaynikam
Copy link
Contributor Author

abhaynikam commented Dec 15, 2019

@vinistock @kaspth Could you please review the changes? I tested it on one of the development app. The rake task is shown in the list of tasks(rake -T) and also run properly.

actiontext/lib/tasks/actiontext.rake Outdated Show resolved Hide resolved
task install: %w( environment run_action_text_generator)

task :run_action_text_generator do
system "#{RbConfig.ruby} ./bin/rails generate action_text:install"
Copy link
Contributor

@kaspth kaspth Dec 15, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm, this doesn't seem right to me. Though I can see we're doing the same thing in the Action Mailbox install Rakefile.

Can we require the correct generator and call start on it like we do in the credentials command? (Assuming that'll still trigger the test framework hook.)

@y-yagi would you happen to have any opinions here?

Copy link
Contributor Author

@abhaynikam abhaynikam Dec 15, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tried requiring the generator and running it. The test framework hook didn't get invoked.

Copy link
Contributor

@vinistock vinistock Dec 15, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree that we shouldn't run it from the bin file. Would this method be of help in this case?

Copy link
Contributor

@kaspth kaspth Dec 15, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That method is only to be run within a generator, not from a Rake task.

Copy link
Contributor

@vinistock vinistock Dec 16, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see. How about this one? Could that be invoked from a rake task?

Copy link
Contributor

@kaspth kaspth Dec 16, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Then I'd prefer to just do something like:

task "action_text:install" do
  require "rails/command" # Can't remember the require path or if Rake tasks already have access to Rails::Command.
  Rails::Command.invoke "generate action_text:install"
end

Copy link
Contributor Author

@abhaynikam abhaynikam Dec 16, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did the necessary changes. Removing desc hide a generator from the list.

@kaspth
Copy link
Contributor

kaspth commented Dec 15, 2019

We probably also want to ensure we don't show the action_text:install generate task when printing every generator.

There's also an opportunity to convert Action Mailbox to use an install generator as well, after this.

@kaspth
Copy link
Contributor

kaspth commented Dec 15, 2019

@vinistock from the users perspective, the fact that we accomplish the install logic via a generator is incidental (e.g. we just switched to it) and the public installer interface is bin/rails action_text:install.

@abhaynikam abhaynikam force-pushed the 35085-update-the-action-text-installer branch from fb6248c to 5013216 Compare Dec 15, 2019
@vinistock
Copy link
Contributor

vinistock commented Dec 15, 2019

@kaspth that makes sense. How does one hide a generator from the list?

@kaspth
Copy link
Contributor

kaspth commented Dec 15, 2019

Can't remember, I'm assuming @abhaynikam can look at other generators or a method called something like printing_generators where the generators are printed for ideas.

@abhaynikam abhaynikam force-pushed the 35085-update-the-action-text-installer branch from 5013216 to 22bf955 Compare Dec 16, 2019
@kaspth
Copy link
Contributor

kaspth commented Dec 16, 2019

@abhaynikam looking better! Surprised we have access to Rails::Command out of the box. Makes sense if people are running bin/rails action_text:install but is that also true for rake action_text:install?

About desc, we want bin/rails action_text:install to show up when printing bin/rails --help removing desc breaks that. It's the action_text:install that we want to hide when doing bin/rails generate --help, not the command (otherwise why bother adding the command?).

… Forwards the installer to run new ActionText generator
@abhaynikam abhaynikam force-pushed the 35085-update-the-action-text-installer branch from 22bf955 to d0c73b3 Compare Dec 16, 2019
@rails-bot rails-bot bot added the railties label Dec 16, 2019
@abhaynikam
Copy link
Contributor Author

abhaynikam commented Dec 16, 2019

@kaspth

but is that also true for rake action_text:install?

Yep. Works with rake action_text:install as well.

About desc, we want bin/rails action_text:install to show up when printing bin/rails --help removing desc breaks that. It's the action_text:install that we want to hide when doing bin/rails generate --help, not the command

Sorry, I misunderstood. I have fixed it in the latest commit. Thanks

@vinistock
Copy link
Contributor

vinistock commented Dec 16, 2019

It's looking good to me!

@kaspth kaspth merged commit 538424a into rails:master Dec 16, 2019
1 check passed
@kaspth
Copy link
Contributor

kaspth commented Dec 16, 2019

@abhaynikam perfect, thanks so much! @vinistock thanks for the extra eyes too. 😄🙏 to you both

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.

None yet

3 participants