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

Enhancement for session binding #198

Merged
merged 3 commits into from
May 26, 2016

Conversation

sh19910711
Copy link

@sh19910711 sh19910711 commented May 25, 2016

This pull request is to find a initial binding for each session. We use the first binding that is in Rails.root as the initial binding.

Issue

From #194:

This often put is in a situation where an error is raised inside of framework code that is invoked from an application code. The error the user sees looks like it's raised from the application code (as this is the piece of code the error page shows), while the binding we have for the console is deep inside the framework code and it looks like Web Console "doesn't work".

Details
  • Find the first binding that is in Rails.root
  • Set stub for Rails.root to return suitable path on testing

Thank you.

@rails-bot
Copy link

r? @gsamokovarov

(@rails-bot has picked a reviewer for you, use r? to override)

@@ -64,6 +64,10 @@ def switch_binding_to(index)

private

def initial_binding
@bindings.find {|b| b.eval('__FILE__').start_with?(Rails.root.to_s) }
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we expect that __FILE__ will always be a string? I think we can hit cases where it is nil for bindings in {instance_,class_,}eval string blocks, so we probably have to do b.eval('__FILE__').to_s in here.

Also a small style nit: { |b| vs {|b|.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I agree with that :-) I will fix them.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated.

@sh19910711 sh19910711 force-pushed the patch/0027/sync-traces-bindings branch from c7e31ce to d60583a Compare May 26, 2016 04:45
@sh19910711 sh19910711 force-pushed the patch/0027/sync-traces-bindings branch from d60583a to 37be341 Compare May 26, 2016 04:47
@gsamokovarov
Copy link
Collaborator

🎊

@gsamokovarov gsamokovarov merged commit 75ba183 into rails:master May 26, 2016
@sh19910711
Copy link
Author

Thanks! 🙇

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants