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 Rails command infrastructure #22457

Merged
merged 1 commit into from Dec 4, 2015
Merged

Add Rails command infrastructure #22457

merged 1 commit into from Dec 4, 2015

Conversation

@ccallebs
Copy link
Contributor

@ccallebs ccallebs commented Nov 30, 2015

Corollary of #21254. Isolates command infrastructure and reduces sprawl of previous PR.

@rails-bot
Copy link

@rails-bot rails-bot commented Nov 30, 2015

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.

@kaspth kaspth assigned kaspth and unassigned arthurnn Dec 1, 2015
@kaspth kaspth added this to the 5.0.0 milestone Dec 1, 2015
@kaspth
kaspth reviewed Dec 1, 2015
View changes
railties/lib/rails/commands/command.rb Outdated
system "rake #{task_name}"
end
end
end

This comment has been minimized.

@kaspth

kaspth Dec 1, 2015
Member

We should only add code we're using. Kill this method 😄

@kaspth
kaspth reviewed Dec 1, 2015
View changes
railties/lib/rails/commands/command.rb Outdated

def tasks
public_instance_methods.map { |method| method.gsub('_', ':') }
end

This comment has been minimized.

@kaspth

kaspth Dec 1, 2015
Member

Kill this too

@kaspth
kaspth reviewed Dec 1, 2015
View changes
railties/lib/rails/commands/command.rb Outdated

def parse_options_for(command_name)
@@command_options.fetch(command_name.to_sym, -> {}).call(@option_parser, @options)
rescue ArgumentError

This comment has been minimized.

@kaspth

kaspth Dec 1, 2015
Member

This shouldn't need to rescue an ArgumentError, why is the above code raising one?

@kaspth
kaspth reviewed Dec 1, 2015
View changes
railties/lib/rails/commands/command.rb Outdated
@@commands << command
end

def command_instance_for(command_name)

This comment has been minimized.

@kaspth

kaspth Dec 1, 2015
Member

Don't like the instance suffix, sounds like it's exposing too much of the methods innards. This should just be command_for

@kaspth
kaspth reviewed Dec 1, 2015
View changes
railties/lib/rails/commands/command.rb Outdated

def self.name_for(task_name)
task_name.gsub(':', '_')
end

This comment has been minimized.

@kaspth

kaspth Dec 1, 2015
Member

This should be named command_name_for. You're calling to_sym on the return value in other place, so let's just have this return a symbol.

@kaspth
kaspth reviewed Dec 1, 2015
View changes
railties/lib/rails/commands/command.rb Outdated

def command_instance_for(command_name)
klass = @@commands.find do |command|
command_instance_methods = command.public_instance_methods

This comment has been minimized.

@kaspth

kaspth Dec 1, 2015
Member

Inline this variable.

@kaspth
kaspth reviewed Dec 1, 2015
View changes
railties/lib/rails/commands/command.rb Outdated
end

def run(task_name)
command_name = Command.name_for(task_name)

This comment has been minimized.

@kaspth

kaspth Dec 1, 2015
Member

Prefer self.class to Command

@kaspth
kaspth reviewed Dec 1, 2015
View changes
railties/lib/rails/commands/command.rb Outdated
command_instance_methods.include?(command_name.to_sym)
end

klass.new(@argv)

This comment has been minimized.

@kaspth

kaspth Dec 1, 2015
Member

klass isn't guaranteed to be set here, we need to check if it's set first.

This comment has been minimized.

@kaspth

kaspth Dec 1, 2015
Member

This is also causing Active Job to fail: https://travis-ci.org/rails/rails/jobs/94034953#L461

@kaspth
kaspth reviewed Dec 1, 2015
View changes
railties/lib/rails/commands/command.rb Outdated
def exists?(task_name)
command_name = Command.name_for(task_name)
!command_instance_for(command_name).nil?
end

This comment has been minimized.

@kaspth

kaspth Dec 1, 2015
Member

Don't like this instance method nestled in with the other class methods. Let's put it below them.

@kaspth
Copy link
Member

@kaspth kaspth commented Dec 1, 2015

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

@kaspth
Copy link
Member

@kaspth kaspth commented Dec 1, 2015

Also thanks for working on this! ❤️

@ccallebs
Copy link
Contributor Author

@ccallebs ccallebs commented Dec 1, 2015

@kaspth No problem! Sorry it's taken so long, and I appreciate the patience.

@ccallebs
ccallebs reviewed Dec 4, 2015
View changes
railties/test/commands/command_test.rb Outdated
Dir.chdir(app_path) { `bin/rails #{task}` }
end

def create_command(class_name: :test, task:)

This comment has been minimized.

@ccallebs

ccallebs Dec 4, 2015
Author Contributor

@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.

This comment has been minimized.

@kaspth

kaspth Dec 4, 2015
Member

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) .

This comment has been minimized.

@ccallebs

ccallebs Dec 4, 2015
Author Contributor

@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?

This comment has been minimized.

@kaspth

kaspth Dec 4, 2015
Member

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 😁

@rafaelfranca
Copy link
Member

@rafaelfranca rafaelfranca commented Dec 4, 2015

r? @kaspth

@kaspth
kaspth reviewed Dec 4, 2015
View changes
railties/lib/rails/commands/command.rb Outdated
klass.new(@argv)
rescue
nil
end

This comment has been minimized.

@kaspth

kaspth Dec 4, 2015
Member

This blanket rescue shouldn't be needed. Why is it needed currently? 😁

@kaspth
Copy link
Member

@kaspth kaspth commented Dec 4, 2015

@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 ❤️

@ccallebs
Copy link
Contributor Author

@ccallebs ccallebs commented Dec 4, 2015

@kaspth Will do, just had one more question. Anything more will be addressed today, I won't let this linger any longer.

@ccallebs
Copy link
Contributor Author

@ccallebs ccallebs commented Dec 4, 2015

@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.

kaspth added a commit that referenced this pull request Dec 4, 2015
Add Rails command infrastructure
@kaspth kaspth merged commit 6616724 into rails:master Dec 4, 2015
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@kaspth
Copy link
Member

@kaspth kaspth commented Dec 4, 2015

Awesome work! ❤️

@ccallebs
Copy link
Contributor Author

@ccallebs ccallebs commented Dec 4, 2015

@kaspth It's been a long crazy trip! :)

@kaspth
Copy link
Member

@kaspth kaspth commented Dec 4, 2015

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`

This comment has been minimized.

@ybart

ybart Dec 7, 2015

This assignation is unneeded

This comment has been minimized.

@josephgrossberg

josephgrossberg Dec 7, 2015

Duplicate line, yes?

This comment has been minimized.

@kaspth

kaspth Dec 7, 2015
Member

Cleared it up with some comments: 2af9c08 😁

@rafaelfranca rafaelfranca modified the milestones: 5.0.0 [temp], 5.0.0 Dec 30, 2015
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

7 participants
You can’t perform that action at this time.