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

move Rails console top level methods to IRB context #3509

Merged
merged 2 commits into from Nov 9, 2011

Conversation

Projects
None yet
4 participants
@amatsuda
Member

amatsuda commented Nov 4, 2011

Currently, methods available in Rails console (such as app, helper, reload!, etc.) are defined on Ruby's top level.
That means, these methods are injected into all Objects. In fact, we can do something funny like this on our Rails console.

> puts 'foobar'.send(:app)
#<ActionDispatch::Integration::Session:0x007ff67c0f8f28>
> puts nil.send(:controller)
#<ApplicationController:0x007ff681bdf5e0>

Reading IRB code, I found the IRB way is, to define this sort of methods in a module named IRB::ExtendCommandBundle, then IRB automatically includes (extends) these methods in IRB console's context.
For example, you see, you can call irb_jobs method in your IRB but the following code raises NoMethodError: undefined methodirb_jobs'`, because this method is mixed into IRB's "main" context via IRB::ExtendCommandBundle.

Object.new.send(:irb_jobs)

Attached a patch and tests for Rails console to follow this IRB way. I confirmed it runs on MRI 1.8, 1.9, JRuby and Rubinius.

@josevalim

This comment has been minimized.

Contributor

josevalim commented Nov 4, 2011

Thanks and I agree that adding such methods to all objects is not a good idea, but would those changes also work with pry? Unlikely, right?

@amatsuda

This comment has been minimized.

Member

amatsuda commented Nov 4, 2011

I don't know. I don't use Pry for Rails projects. If Pry wants to support Rails, maybe they aught to do something better than this (this insane script is linked from the readme. I don't think they're serious so far).

Anyway, if you think Rails should be flexible enough so that users can choose their own console backend, probably I should better not directly monkey patch IRB::ExtendCommandBundle but create a new module defining app, helpers etc. and mix it into IRB::ExtendCommandBundle.
If you prefer that way, I'll make another patch.

@sarahhodne

This comment has been minimized.

Contributor

sarahhodne commented Nov 5, 2011

This is how I use pry for the rails console. I'll test if this works on pry as soon as I'm done running the rails tests.

@sarahhodne

This comment has been minimized.

Contributor

sarahhodne commented Nov 5, 2011

Nope, this doesn't work with (my setup of) pry. Would a better way be to put it in a Rails-specific module and include it into IRB however IRB does it when you run rails console and leave it to the user for pry? So for Pry I would do include Rails::Console::Helpers in my .pryrc file?

@ghost

This comment has been minimized.

ghost commented Nov 9, 2011

👍 @dvyjones.
I'm also using that same example.

If Pry could have a module it could include from, it could also be easy to write a Pry plugin that made those methods available as Pry commands.

@amatsuda

This comment has been minimized.

Member

amatsuda commented Nov 9, 2011

@dvyjones @robgleeson Thank you very much for your feedbacks :)

Externalized the console helper methods into a module named Rails::ConsoleMethods so that Pry users can easily mix them into your console. Hope this works fine.

@josevalim

This comment has been minimized.

Contributor

josevalim commented Nov 9, 2011

Sounds good. Thank everyone for your work, merging.

josevalim added a commit that referenced this pull request Nov 9, 2011

Merge pull request #3509 from amatsuda/console_extend_command_bundle
move Rails console top level methods to IRB context

@josevalim josevalim merged commit 4857339 into rails:master Nov 9, 2011

@guilleiguaran

This comment has been minimized.

Member

guilleiguaran commented Nov 9, 2011

There is a broken test with this, can you take a look to it?:

1) Error:
test_console_block_is_executed_when_MyApp.load_console_is_called(RailtiesTest::RailtieTest):

ActiveSupport::Testing::RemoteError: caught NameError: uninitialized constant Rails::Application::IRB
/home/vagrant/builds/rails/rails/railties/lib/rails/application.rb:209:in `initialize_console'
/home/vagrant/builds/rails/rails/railties/lib/rails/application.rb:108:in `load_console'
/home/vagrant/builds/rails/rails/railties/lib/rails/railtie/configurable.rb:30:in `send'
/home/vagrant/builds/rails/rails/railties/lib/rails/railtie/configurable.rb:30:in `method_missing'

test/railties/railtie_test.rb:162:in `test_console_block_is_executed_when_MyApp.load_console_is_called'
@amatsuda

This comment has been minimized.

Member

amatsuda commented Nov 9, 2011

Ugh. Sorry for that. Made another push on this branch, but it seems a new commit doesn't appear on an already merged pull request, so I made another pr #3589.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment