Add Rails command infrastructure #22457
Conversation
|
Thanks for the pull request, and welcome! The Rails team is excited to review your changes, and you should hear from @arthurnn (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. |
| system "rake #{task_name}" | ||
| end | ||
| end | ||
| end |
There was a problem hiding this comment.
We should only add code we're using. Kill this method 😄
|
Don't worry about the documentation for now, I'll write it after this is merged. We also need some tests for the general functionality here. Some simple integration tests modeled after https://github.com/rails/rails/blob/master/railties/test/application/test_runner_test.rb would do. The helpers you'll need are here: https://github.com/rails/rails/blob/master/railties/test/isolation/abstract_unit.rb |
|
Also thanks for working on this! ❤️ |
|
@kaspth No problem! Sorry it's taken so long, and I appreciate the patience. |
| Dir.chdir(app_path) { `bin/rails #{task}` } | ||
| end | ||
|
|
||
| def create_command(class_name: :test, task:) |
There was a problem hiding this comment.
@kaspth I think we're going to have a problem testing here -- I see no way given the way commands are currently loaded to "inject" new ones into the process. My idea was to create an lib/commands directory (similar to lib/tasks, but for these commands) . That's a pretty big decision though, so I wanted to get feedback. If you can think of a way to dynamically create commands, let me know and this becomes a non-issue.
There was a problem hiding this comment.
What do you mean by new ones? We don't have to generate a new command, it's fine to just use one of the existing ones (which will be the dev_cache command because it's the only one that exists currently) .
There was a problem hiding this comment.
@kaspth I guess I misunderstood then -- I thought you just wanted generic tests surrounding the addition of new commands. What would be different about these tests vs. the dev_cache tests below?
There was a problem hiding this comment.
Just that we are exercising the command running code a bit. But then again I guess any behavior is tested through the commands we ship with.
Alright, if all the Railties tests pass, then it's fine. I'll look into what other tests need after this then 😁
|
r? @kaspth |
| klass.new(@argv) | ||
| rescue | ||
| nil | ||
| end |
There was a problem hiding this comment.
This blanket rescue shouldn't be needed. Why is it needed currently? 😁
|
@ccallebs we don't have much time left to get this in. Please address my last comments as soon as you can and squash your commits down to one. Then I'll merge and we can start transforming more tasks to commands ❤️ |
|
@kaspth Will do, just had one more question. Anything more will be addressed today, I won't let this linger any longer. |
|
@kaspth Everything is addressed and squashed! Let me know if you need anything else. I'll jump in to help with some of the other commands once this is merged. |
Add Rails command infrastructure
|
Awesome work! ❤️ |
|
@kaspth It's been a long crazy trip! :) |
|
And we ain't out of the woods yet ;) |
|
|
||
| test 'dev:cache deletes file and outputs message' do | ||
| Dir.chdir(app_path) do | ||
| output = `rails dev:cache` |
There was a problem hiding this comment.
Cleared it up with some comments: 2af9c08 😁
Corollary of #21254. Isolates command infrastructure and reduces sprawl of previous PR.