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

Fix exception output capturing #482

Merged
merged 1 commit into from
Oct 16, 2024
Merged

Conversation

andyw8
Copy link
Contributor

@andyw8 andyw8 commented Oct 16, 2024

Closes https://github.com/Shopify/team-ruby-dx/issues/1297

This ensure the failure output can be displayed in the Ruby LSP output panel.

To 🎩 :

  • dev down on Core
  • Save a model to trigger and RBI update
  • Observe that the Ruby LSP output panel shows the appropriate error.

Note: Failures that were previously silent, such as hovering over a ActiveRecord model when the DB isn't set up, will now show in the logs.

@andyw8 andyw8 added the bugfix This PR fixes an existing bug label Oct 16, 2024
@andyw8 andyw8 force-pushed the andy8/fix-exception-output-capturing branch from c987284 to d1f04bc Compare October 16, 2024 16:35
rescue => e
send_message({ error: e.full_message(highlight: false) })
log_message("Request #{request_name} failed:\n" + e.full_message(highlight: false))
Copy link
Contributor Author

@andyw8 andyw8 Oct 16, 2024

Choose a reason for hiding this comment

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

@vinistock I updated this so that we show the request name for both rescue clauses.

Copy link
Member

Choose a reason for hiding this comment

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

Sounds good!

@@ -119,12 +119,17 @@ def execute(request, params)
require params[:server_addon_path]
ServerAddon.finalize_registrations!(@stdout)
when "server_addon/delegate"
server_addon_name = params.delete(:server_addon_name)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We need to avoid deleting the params here since they may be needed in the error message.

rescue => e
send_message({ error: e.full_message(highlight: false) })
log_message("Request #{request_name} failed:\n" + e.full_message(highlight: false))
Copy link
Contributor Author

Choose a reason for hiding this comment

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

log_message just writes to $stderr, which we are already capturing.

@andyw8
Copy link
Contributor Author

andyw8 commented Oct 16, 2024

FYI @KaanOzkan

@andyw8 andyw8 marked this pull request as ready for review October 16, 2024 16:45
@andyw8 andyw8 requested a review from a team as a code owner October 16, 2024 16:45
@andyw8 andyw8 merged commit 0c98405 into main Oct 16, 2024
28 checks passed
@andyw8 andyw8 deleted the andy8/fix-exception-output-capturing branch October 16, 2024 16:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bugfix This PR fixes an existing bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants