introduce KyoException #929
Conversation
fwbrasil
force-pushed
the
kyo-exception
branch
from
54e30aa to
3f691a2
Compare
| class KyoException private[kyo] ( | ||
| message: Text = null, | ||
| cause: Text | Throwable = null | ||
| )(using val frame: Frame) extends NoStackTrace: |
There was a problem hiding this comment.
| )(using val frame: Frame) extends NoStackTrace: | |
| )(using val frame: Frame) extends Exception(message.toString, cause match { case cause: Throwable => cause; case _ => null }) with NoStackTrace: |
Would it make sense to make this a subclass of Exception? The docs says about Exception (and it's subclasses)
indicates conditions that a reasonable application might want to catch
Is that the case here?
There was a problem hiding this comment.
I think you can also avoid the NoStackTrace mixin (though perhaps it's useful for users to see) via an argument to Exception/Throwable's constructor.
There was a problem hiding this comment.
I've added extends Exception but kept the method overrides and NoStackTrace. We need to override getCause anyway to avoid synchronization (#331) and the logic to handle non-Throwable causes seems more opaque as constructor parameters.
We also need NoStackTrace to signal to the kernel that it shouldn't try to enrich the stack trace.
| case cause: Text @unchecked => Maybe(cause) | ||
|
|
||
| val msg = frame.render(("⚠️ KyoException".red.bold :: Maybe(message).toList ::: detail.toList).map(_.show)*) | ||
| s"\n$msg\n" |
There was a problem hiding this comment.
Are the two \n really supposed to be there? Shouldn't formatting like this be potentially done by the users of getMessage instead of in getMessage itself?
There was a problem hiding this comment.
That's to provide better rendering in the console otherwise the top line gets in the same line as the exception type. I imagine we'll eventually need to provide more customization of this behavior since verbose logging during an error state can contribute to degradation but, for now, I think the better error reporting will be valuable to people trying out the library. Example output:
There was a problem hiding this comment.
provide better rendering in the console
Yeah, totally. But my point was that it seems like a concert of the code which does the rendering/printing/formatting of an exception to standard output. getMessage is generally implemented in a way which is independent/oblivious whether the target is standard output, HTML, JSON or whatever else.
It's a very small thing, so I don't mind at all, but it caught my attention, so I've mentioned it. Feel free to ignore 🙂
| case _: Throwable => Absent | ||
| case cause: Text @unchecked => Maybe(cause) | ||
|
|
||
| val msg = frame.render(("⚠️ KyoException".red.bold :: Maybe(message).toList ::: detail.toList).map(_.show)*) |
There was a problem hiding this comment.
it may be easier to use .getOrElse("").show to avoid the list?
There was a problem hiding this comment.
The size of the list determines how many lines will be rendered
No description provided.