-
-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
Consider removing attemptNarrow #3360
Comments
This is partially addressed by #3361. In the longer run we should consider what to do with |
I can't find any mentions of this on GitHub so, on the topic of import cats.data.Validated
def boom = throw new OutOfMemoryError
Validated.catchOnly[Error] { boom } // => Invalid(java.lang.OutOfMemoryError): Validated[Error,Nothing] Looking at the hierarchy, it looks like the list of problematic type parameters would be limited to fatal exceptions themselves, which is obvious, but also |
@travisbrown please consider keeping I found this method pretty convenient when you need to temporary put your business error from |
@lukoyanov The current plan I think is to keep it but with a |
@travisbrown |
Somehow I'd never paid attention to
attemptNarrow
, which we added in 2.1.0. It has some problems. For example it's trivially easy to make it crash at runtime:It can also be used to launder a runtime type check:
Maybe that's fine, and you should know that if you've got a
ClassTag
around you've given up any hope of parametricity, but that seems pretty far from the spirit of Cats.@tpolecat requested tests for the erasure issues in the original PR, but unfortunately the tests that were added don't actually check for the relevant problems.
I guess the reasoning is that
E
is usually an exception type, and generally won't be generic? In any case I think we should make both the type class and syntax methods package-private in future 2.x releases and remove them in 3.0, or at least put an explicit<: Throwable
constraint on them (which doesn't eliminate the possibility of runtime crashes, but does make them much less likely).The text was updated successfully, but these errors were encountered: