safer_rails_console

safer_rails_console copied to clipboard

I just got this error while using safer_rails_console today in development:

NameError: uninitialized constant SaferRailsConsole::Patches::Sandbox::AutoRollback::RED
from /home/cbliard/.rbenv/versions/2.5.3/lib/ruby/gems/2.5.0/gems/safer_rails_console-0.3.0/lib/safer_rails_console/patches/sandbox/auto_rollback.rb:15:in `handle_and_reraise_exception'

This PR should fix the error.

Tests added:

::SaferRailsConsole::Patches::Sandbox::AutoRollback
  .handle_and_reraise_exception
    when raising a PG::ReadOnlySqlTransaction exception
      outputs a message on stdout and forwards the exception
    when raising a classic exception
      rollbacks, begins a new transaction and forwards the exception

Please note that the first test in spec/safer_rails_console/console_spec.rb only passes because it is run before the tests I just added: I needed to load ::SaferRailsConsole::Patches::Sandbox::AutoRollback to test handle_and_reraise_exception method, but it makes the test "SaferRailsConsole::Console.initialize_sandbox loads sandbox patches" fail if it is run afterwards (Because it checks that the patch is not loaded before calling initialize_sandbox). That makes the test order-dependent and that's bad. Happy to fix it if you have some guidance here.

I will do the proposed changes. Expect it on next Friday. Thanks for the feedback.

I dug a little deeper about this issue, trying to implement it as an integration test. In fact it passes: the constant RED is defined in this case.

It's because the SaferRailsConsole::Colors is included in lib/safer_rails_console/consoles/irb.rb and makes RED available as a constant in the global scope.

I think I got the undefined constant RED because we are using pry for rails console in development and test (and irb in prod). With pry the file is not included. It's indicated in the README but I did not notice.

So I am not sure if my fix is relevant anymore. For me it looks like using safer_rails_console gem should not add color_text, RED, GREEN, and others to the global namespace. So the include SaferRailsConsole::Colors should be removed from lib/safer_rails_console/irb.rb and it should use the entire namespace, like SaferRailsConsole::Colors.color_text(text, SaferRailsConsole::Colors::RED).

What are your thoughts about it?

Happy new year - apologies for the delay in response.

Thanks for taking a deeper look into it. I agree with not polluting the global namespace. The followup is to also generate a pry.rb, that would change the prompt similarly per _pry_.config.command_prefix = "%". The console configuration loading will also need to get updated so it loads the correct file (irb.rb vs pry.rb) given the REPL used - https://github.com/salsify/safer_rails_console/blob/master/lib/safer_rails_console/console.rb#L14 . I can add that in when I get a chance in the next week or two, unless you want to take a stab at it.

Happy new year to you too!

I'll try to get a look this week. It should be easier for me as I am already using pry and can reproduce the issue at will. I'll try to add an integration test using pry too.

can we merge it?