Throwable toString override #1263
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
daddykotex
changed the title
7 tasks
lewisjkl
approved these changes
Oct 16, 2023
daddykotex
force-pushed
the
dfrancoeur/0.18-throwable-tostring
branch
from
c5a55ef
to
fd94ab7
Compare
Baccata
reviewed
Oct 31, 2023
| @@ -801,6 +802,10 @@ private[internals] class Renderer(compilationUnit: CompilationUnit) { self => | |||
| ) | |||
| } | |||
|
|
|||
| // Default toString on Throwable prints the classname, instead we use the case class toString. | |||
| private def defaultThrowableGetMessage = | |||
| line"override def toString(): $string_ = $scalaRuntime._toString(this)" | |||
There was a problem hiding this comment.
Sorry for reviewing this only now. Anyway, the documentation of the ScalaRunTime object states the following :
/** The object ScalaRunTime provides support methods required by
* the scala runtime. All these methods should be considered
* outside the API and subject to change or removal without notice.
*/therefore, I'm not comfortable with this implementation. Can you elaborate on the rationale for wanting to use this ? I guess it's just for the convenience of it.
In any case, we can render an actual implementation that'd be using a StringBuilder quite easily.
There was a problem hiding this comment.
I missed that completely, that's fair.
In any case, we can render an actual implementation that'd be using a StringBuilder quite easily.
Any format we prefer? For Foo(bar: String), default implementation renders Foo(barValue), we can choose how to render if we implement it ourselves
This reverts commit 831d99c.
Conflicts: CHANGELOG.md
I chose to implement toString, because implementing getMessage would lead to `smithy4s.example.ClientError: smithy4s.example.ClientError(400, "oops")` which felt weird
daddykotex
force-pushed
the
dfrancoeur/0.18-throwable-tostring
branch
from
e886e5d
to
2f51c44
Compare
Baccata
approved these changes
Nov 2, 2023
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Default
toStringonThrowableprints the classname. This is suboptimal when we could have the case classtoStringimplementation available.PR Checklist (not all items are relevant to all PRs)