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

Display nested exceptions on the debug view #32410

Merged
merged 1 commit into from Jul 20, 2018

Conversation

Projects
None yet
6 participants
@yuki24
Contributor

yuki24 commented Apr 2, 2018

This PR adds the ability to show nested exceptions, which I proposed a few weeks ago on the core mailing list.

It's probably easier to understand when you actually see it in action rather than explaining the entire thing, so here comes a screenshot:

out

I'm going to add the same functionality to the plain text response once this is merged.

@rails-bot

This comment has been minimized.

Show comment
Hide comment
@rails-bot

rails-bot Apr 2, 2018

r? @georgeclaghorn

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

rails-bot commented Apr 2, 2018

r? @georgeclaghorn

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

@kaspth

I do like the idea of this. But how common are exception causes in development?

end
def source_to_show_id
(traces[trace_to_show].first || {})[:id]

This comment has been minimized.

@kaspth

kaspth Apr 2, 2018

Member

Hash(traces[trace_to_show].first)[:id]?

@kaspth

kaspth Apr 2, 2018

Member

Hash(traces[trace_to_show].first)[:id]?

This comment has been minimized.

@yuki24

yuki24 Apr 2, 2018

Contributor

Possible, but I couldn't come up with strong advantages over (... || {}) (very basic syntax that beginners can understand while Hash(...) is a method call that could be overridden)

@yuki24

yuki24 Apr 2, 2018

Contributor

Possible, but I couldn't come up with strong advantages over (... || {}) (very basic syntax that beginners can understand while Hash(...) is a method call that could be overridden)

Show outdated Hide outdated ...ck/lib/action_dispatch/middleware/templates/rescues/diagnostics.html.erb
Show outdated Hide outdated ...ck/lib/action_dispatch/middleware/templates/rescues/diagnostics.html.erb
Show outdated Hide outdated .../action_dispatch/middleware/templates/rescues/invalid_statement.html.erb
Show outdated Hide outdated ...b/action_dispatch/middleware/templates/rescues/missing_template.html.erb
@@ -1,52 +1,54 @@
<% names = @traces.keys %>
<% names = traces.keys %>

This comment has been minimized.

@kaspth

kaspth Apr 2, 2018

Member

With this we could spare the multiple instances for error_index: 0 in the render calls.

<% error_index = local_assigns.fetch(:error_index, 0) %>
@kaspth

kaspth Apr 2, 2018

Member

With this we could spare the multiple instances for error_index: 0 in the render calls.

<% error_index = local_assigns.fetch(:error_index, 0) %>
@yuki24

This comment has been minimized.

Show comment
Hide comment
@yuki24

yuki24 Apr 2, 2018

Contributor

how common are exception causes in development?

Any exception that originals from a rescue_from block has a cause. Also, almost any network-related exception has a cause when retry or some sort of re-connecting happens in a rescue block.

Contributor

yuki24 commented Apr 2, 2018

how common are exception causes in development?

Any exception that originals from a rescue_from block has a cause. Also, almost any network-related exception has a cause when retry or some sort of re-connecting happens in a rescue block.

@yuki24

This comment has been minimized.

Show comment
Hide comment
@yuki24

yuki24 Apr 4, 2018

Contributor

slightly updated the PR.

Contributor

yuki24 commented Apr 4, 2018

slightly updated the PR.

Show outdated Hide outdated actionpack/lib/action_dispatch/middleware/exception_wrapper.rb
@gsamokovarov

This comment has been minimized.

Show comment
Hide comment
@gsamokovarov

gsamokovarov Apr 15, 2018

Contributor

@yuki24 you may also wanna look into making a change in rails/web-console as it relies on the structure of the error page.

Contributor

gsamokovarov commented Apr 15, 2018

@yuki24 you may also wanna look into making a change in rails/web-console as it relies on the structure of the error page.

@kaspth

I think this is pretty much good to go. @yuki24 can you check what this would mean for web-console?

Show outdated Hide outdated actionpack/lib/action_dispatch/middleware/exception_wrapper.rb
@yuki24

This comment has been minimized.

Show comment
Hide comment
@yuki24

yuki24 Jul 6, 2018

Contributor

@kaspth I haven't looked into it yet will do so if I could find time this weekend.

Contributor

yuki24 commented Jul 6, 2018

@kaspth I haven't looked into it yet will do so if I could find time this weekend.

@yuki24

This comment has been minimized.

Show comment
Hide comment
@yuki24

yuki24 Jul 9, 2018

Contributor

@gsamokovarov @kaspth here is a proof-of-concept branch: rails/web-console@master...yuki24:support-for-nested-exceptions

The current web-conole only points to the last exception. It'd have to be able to switch the exception to look at so it could properly switch the the binding. There's no test yet, but you should be able to see how it works and also how much of complexity needs to be added for nested exception support.

Contributor

yuki24 commented Jul 9, 2018

@gsamokovarov @kaspth here is a proof-of-concept branch: rails/web-console@master...yuki24:support-for-nested-exceptions

The current web-conole only points to the last exception. It'd have to be able to switch the exception to look at so it could properly switch the the binding. There's no test yet, but you should be able to see how it works and also how much of complexity needs to be added for nested exception support.

@kaspth

This comment has been minimized.

Show comment
Hide comment
@kaspth

kaspth Jul 15, 2018

Member

@yuki24 that branch looks doable to me 😊 — for this PR have you tried @matthewd's wrapped causes suggestion yet? 😉

@gsamokovarov how does that seem to you? I'd like to move forward with this in the meantime 😊

Member

kaspth commented Jul 15, 2018

@yuki24 that branch looks doable to me 😊 — for this PR have you tried @matthewd's wrapped causes suggestion yet? 😉

@gsamokovarov how does that seem to you? I'd like to move forward with this in the meantime 😊

@gsamokovarov

This comment has been minimized.

Show comment
Hide comment
@gsamokovarov

gsamokovarov Jul 15, 2018

Contributor

@kaspth, please do. @yuki24 I have a few comments about the code structure, but feel free to open the PR and we'll work it out on the rails/web-console side.

Contributor

gsamokovarov commented Jul 15, 2018

@kaspth, please do. @yuki24 I have a few comments about the code structure, but feel free to open the PR and we'll work it out on the rails/web-console side.

@yuki24

This comment has been minimized.

Show comment
Hide comment
@yuki24

yuki24 Jul 16, 2018

Contributor

@kaspth yes, I like @matthewd's idea. I'll update the PR to use it.

@gsamokovarov I'll send a PR shortly.

Contributor

yuki24 commented Jul 16, 2018

@kaspth yes, I like @matthewd's idea. I'll update the PR to use it.

@gsamokovarov I'll send a PR shortly.

@yuki24 yuki24 referenced this pull request Jul 16, 2018

Open

WIP: Add support for nested exceptions #265

0 of 1 task complete
def wrapped_causes_for(exception, backtrace_cleaner)
causes_for(exception).map { |cause| self.class.new(backtrace_cleaner, cause) }
end

This comment has been minimized.

@kaspth

kaspth Jul 20, 2018

Member

Man, this looks clean now 😍

@kaspth

kaspth Jul 20, 2018

Member

Man, this looks clean now 😍

@kaspth kaspth merged commit ba78b1f into rails:master Jul 20, 2018

2 checks passed

codeclimate All good!
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@kaspth

This comment has been minimized.

Show comment
Hide comment
@kaspth

kaspth Jul 20, 2018

Member

Well done, thanks! 👏

Member

kaspth commented Jul 20, 2018

Well done, thanks! 👏

@yuki24 yuki24 deleted the yuki24:show-cause-on-debug-view-2 branch Jul 20, 2018

@yuki24 yuki24 changed the title from Show nested exceptions on the debug view to Display nested exceptions on the debug view Oct 2, 2018

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