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

Make a common public parent for raise exception #3349

Merged
merged 6 commits into from
Jan 19, 2024

Conversation

nomisRev
Copy link
Member

No description provided.

@kyay10
Copy link
Collaborator

kyay10 commented Jan 17, 2024

What type of code is this PR intended to allow? merely distinguishing Raise errors from other cancellations, or is it something more? I ask because raised and raise are both internal, and I'm thinking that potentially exposing them could be useful for debugging such cases or logging them. Alternatively, we might want to add a better toString for the exception to include the raised and raise.

@nomisRev
Copy link
Member Author

Yes, the only goal was merely distinguishing Raise errors from other cancellations as we've discussed in your CCE PR.

I ask because raised and raise are both internal, and I'm thinking that potentially exposing them could be useful for debugging such cases or logging them. Alternatively, we might want to add a better toString for the exception to include the raised and raise.

I didn't want to prematurely add it, since there are safer and better ways to access those values. Such as traced, recover, attempt if its get added, etc.

It'll be easy to expose it later on, but hard/impossible to remove.

@kyay10
Copy link
Collaborator

kyay10 commented Jan 18, 2024

I like this change because it finally allows the usage intended by the try-catch tests.
Should we update the tests to reflect that? I think now they're catching general Throwable, so perhaps we can change it to RaiseCancellationException.

@nomisRev
Copy link
Member Author

Yes, to both your comments @kyay10! Will update the PR momentarily 👍

@nomisRev
Copy link
Member Author

@kyay10 I added your suggestions.

The message of the exception always includes:

internal const val RaiseCancellationExceptionCaptured: String =
  "kotlin.coroutines.cancellation.CancellationException should never get swallowed. Always re-throw it if captured." +
    "This swallows the exception of Arrow's Raise, and leads to unexpected behavior." +
    "When working with Arrow prefer Either.catch or arrow.core.raise.catch to automatically rethrow CancellationException."

Copy link
Member

@serras serras left a comment

Choose a reason for hiding this comment

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

Nice!

Copy link
Collaborator

@kyay10 kyay10 left a comment

Choose a reason for hiding this comment

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

LGTM!

@nomisRev nomisRev merged commit d6fb35d into main Jan 19, 2024
11 checks passed
@nomisRev nomisRev deleted the sv-public-raise-exception branch January 19, 2024 15:05
kyay10 added a commit to kyay10/arrow that referenced this pull request Jan 23, 2024
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