Use Thor for built-in tmp task #47698

p8 · 2023-03-16T21:03:32Z

Currently we use both Thor and Rake for bin/rails commands. We eventually want to get all the built-ins task promoted to Thor Commands. This migrates the tmp task to Thor.

As tmp directories are sometimes hidden by editors, I've used temp instead.

Checklist

Before submitting the PR make sure the following are checked:

This Pull Request is related to one change. Changes that are unrelated should be opened in separate PRs.
Commit message has a detailed description of what changed and why. If this PR fixes a related issue include it in the commit message. Ex: [Fix #issue-number]
Tests are added or updated if you fix a bug or add a feature.
CHANGELOG files are updated for the changed libraries if there is a behavior change or additional feature. Minor bug fixes and documentation changes should not be included.

jonathanhefner

As tmp directories are sometimes hidden by editors, I've used temp instead.

I don't think changing the code to suit the editor is the right approach.

Which editor are you seeing this problem in? Would it possible to add a dot file in the command's directory (like a .keep file) to force the editor to recognize the directory?

railties/lib/rails/commands/temp/helpers.rb

jonathanhefner · 2023-03-17T20:10:56Z

railties/lib/rails/commands/temp/temp_command.rb

+        Rails::Command.invoke "tmp:storage:clear"
+      end
+
+      # desc :create, "Create tmp directories for cache, sockets, and pids"


I think we can uncomment this. It's not commented out for the current Rake task.

jonathanhefner · 2023-03-17T20:11:12Z

railties/lib/rails/commands/temp/temp_command.rb

+    module Temp
+      class CacheCommand < Base # :nodoc:


Unfortunately, putting this module in railties/lib/rails/commands/temp/temp_command.rb means lookup won't find these commands (because namespaces_to_paths won't return the right path):

$ bin/rails tmp:screenshots:clear Unrecognized command "tmp:screenshots:clear" (Rails::Command::UnrecognizedCommandError) Did you mean? tmp:sockets:clear $ bin/rails tmp:sockets:clear Unrecognized command "tmp:sockets:clear" (Rails::Command::UnrecognizedCommandError) Did you mean? tmp:cache:clear $ bin/rails tmp:cache:clear Unrecognized command "tmp:cache:clear" (Rails::Command::UnrecognizedCommandError) Did you mean? tmp:clear

(But "Did you mean?" can subsequently suggest them because it uses printing_commands.)

So each command needs to go into a dedicated subdirectory.

I've extracted these to separate files.

jonathanhefner · 2023-03-17T20:36:56Z

railties/test/commands/temp_test.rb

+      FileUtils.mkdir_p("tmp/cache")
+      FileUtils.touch("tmp/cache/cache_file")
+
+      FileUtils.mkdir_p("tmp/sockets")
+      FileUtils.touch("tmp/sockets/socket_file")
+
+      FileUtils.mkdir_p("tmp/screenshots")
+      FileUtils.touch("tmp/screenshots/fail.png")
+
+      FileUtils.mkdir_p("tmp/storage/6h/np")
+      FileUtils.touch("tmp/storage/6h/np/6hnp81jvgt42pcfqtlpoy8qshfb0")


We can use the app_file helper for each of these.

Nice. I've made the change.

jonathanhefner · 2023-03-17T20:39:36Z

railties/test/commands/temp_test.rb

+      assert_not File.exist?("tmp/cache/cache_file")
+      assert_not File.exist?("tmp/sockets/socket_file")
+      assert_not File.exist?("tmp/screenshots/fail.png")
+      assert_not File.exist?("tmp/storage/6h/np/6hnp81jvgt42pcfqtlpoy8qshfb0")


It might be worth adding a private assert_file helper for these that also incorporates app_path calls. Then we can drop the Dir.chdir block.

I've added an assert_dir_cleared that checks the content of the directory.
To make sure we don't delete the directory, this assertion also fails if the directory no longer exists.

jonathanhefner · 2023-03-17T20:43:31Z

railties/test/commands/temp_test.rb

+      rails "tmp:create"
+      assert Dir.exist?("tmp/cache")
+      assert Dir.exist?("tmp/sockets")
+      assert Dir.exist?("tmp/pids")
+      assert Dir.exist?("tmp/cache/assets")


I think it would be good to call FileUtils.rm_rf(tmp_dirs) before rails "tmp:create". That way we can be sure we are actually testing creation.

p8 · 2023-03-18T11:07:47Z

Which editor are you seeing this problem in? Would it possible to add a dot file in the command's directory (like a .keep file) to force the editor to recognize the directory?

I'm using Neovim with NerdTree.
I thought this was a more common problem, but I'm probably mistaken.
I've updated my vim config and renamed the files 😄

railties/lib/rails/commands/tmp/tmp_command.rb

Currently we use both Thor and Rake for `bin/rails` commands. We eventually want to get all the built-ins task promoted to Thor Commands. This migrates the `tmp` task to Thor. Co-authored-by: Juan Vásquez <javasgon@gmail.com> Co-authored-by: Jonathan Hefner <jonathan@hefner.pro>

rails-bot bot added the railties label Mar 16, 2023

p8 force-pushed the railties/thor-tmp-task branch 3 times, most recently from ff8d107 to 119c08c Compare March 16, 2023 21:15

p8 mentioned this pull request Mar 16, 2023

Use Thor for built-in tmp:cache:clear task #47696

Closed

4 tasks

p8 force-pushed the railties/thor-tmp-task branch 4 times, most recently from e4eae66 to 80612bd Compare March 16, 2023 21:44

rails-bot bot added actionmailbox actiontext activestorage labels Mar 16, 2023

p8 force-pushed the railties/thor-tmp-task branch from 80612bd to 7964f65 Compare March 17, 2023 10:19

jonathanhefner reviewed Mar 17, 2023

View reviewed changes

p8 force-pushed the railties/thor-tmp-task branch 4 times, most recently from 714140b to 800f8da Compare March 18, 2023 12:16

JuanVqz reviewed Mar 20, 2023

View reviewed changes

railties/lib/rails/commands/tmp/tmp_command.rb Outdated Show resolved Hide resolved

p8 force-pushed the railties/thor-tmp-task branch from 4bf9d76 to b22a6a7 Compare March 20, 2023 07:10

p8 force-pushed the railties/thor-tmp-task branch from b22a6a7 to d2c075e Compare March 21, 2023 10:55

rails-bot bot added the docs label Mar 21, 2023

skipkayhil mentioned this pull request Apr 27, 2023

tmp:create creates the same dirs that tmp:clear removes #48057

Open

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use Thor for built-in tmp task #47698

Use Thor for built-in tmp task #47698

p8 commented Mar 16, 2023 •

edited

jonathanhefner left a comment

jonathanhefner Mar 17, 2023

p8 Mar 18, 2023

jonathanhefner Mar 17, 2023

p8 Mar 18, 2023

jonathanhefner Mar 17, 2023

p8 Mar 18, 2023

jonathanhefner Mar 17, 2023

p8 Mar 18, 2023

jonathanhefner Mar 17, 2023

p8 Mar 18, 2023

p8 commented Mar 18, 2023

Use Thor for built-in tmp task #47698

Are you sure you want to change the base?

Use Thor for built-in tmp task #47698

Conversation

p8 commented Mar 16, 2023 • edited

Checklist

jonathanhefner left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

p8 commented Mar 18, 2023

p8 commented Mar 16, 2023 •

edited