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

Redirect PSI errors to the default exception handler #111

Merged
merged 9 commits into from
Jan 15, 2025

Conversation

yevhenii-nadtochii
Copy link
Contributor

@yevhenii-nadtochii yevhenii-nadtochii commented Jan 14, 2025

This PR wraps the runnable we give to PSI into a try-catch block, with the configurable possibility to throw right away.

We need this change, so that users of ProtoData could see these errors.

I have tested this approach locally with changes to tool-base and ProtoData, it works. And I have not found any usages of commandProcessor.executeCommand() out of this extension, which we use as an entry point to PSI modifications.

Addresses problem (2) described in this comment.

@yevhenii-nadtochii yevhenii-nadtochii self-assigned this Jan 14, 2025
@yevhenii-nadtochii yevhenii-nadtochii marked this pull request as ready for review January 14, 2025 15:15
@yevhenii-nadtochii
Copy link
Contributor Author

@armiol @alexander-yevsyukov PTAL

Copy link
Contributor

@armiol armiol left a comment

Choose a reason for hiding this comment

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

@yevhenii-nadtochii as discussed vocally, let's adjust both the default behaviour of the exception handler. And the documentation of the public member.

Copy link
Contributor

@alexander-yevsyukov alexander-yevsyukov left a comment

Choose a reason for hiding this comment

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

Please see my comments.

*/
private fun logAndTerminate(t: Throwable) {
t.printStackTrace()
exitProcess(1)
Copy link
Contributor

@alexander-yevsyukov alexander-yevsyukov Jan 14, 2025

Choose a reason for hiding this comment

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

1 should be a constant visible somewhere in the public documentation. Please see how we do it in ProtoData.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

BTW, we probably should use 1 there too because ProtoData usually exits with 255.

errorHandler(t)
}
}
executeSilent(withHandledErrors)
Copy link
Contributor

Choose a reason for hiding this comment

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

This code combination is confusing because it won't result in the really silent execution.

* The default error handler for [execute] that logs the given [Throwable] to [System.err]
* and terminates the currently running process with a non-zero exit code.
*/
private fun logAndTerminate(t: Throwable) {
Copy link
Contributor

Choose a reason for hiding this comment

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

We don't really log here. We print the stacktrace to the console.

* @see execute
*/
@JvmName("executeSilent")
public fun executeSilent(runnable: Runnable) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do we need this, assuming we have the default value for execute?

@yevhenii-nadtochii
Copy link
Contributor Author

@armiol @alexander-yevsyukov PTAL

I've removed usages of the exception handler because we can throw right away now.

Copy link
Contributor

@armiol armiol left a comment

Choose a reason for hiding this comment

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

@yevhenii-nadtochii LGTM from my side.

Copy link
Contributor

@alexander-yevsyukov alexander-yevsyukov left a comment

Choose a reason for hiding this comment

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

LGTM

@yevhenii-nadtochii yevhenii-nadtochii merged commit bea9d54 into master Jan 15, 2025
6 checks passed
@yevhenii-nadtochii yevhenii-nadtochii deleted the redirect-psi-errors branch January 15, 2025 13:22
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