-
-
Notifications
You must be signed in to change notification settings - Fork 249
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
Define an innerThrowable
for ExecutionError
s raised within provided ArgBuilder
s
#2118
Conversation
@@ -12,8 +12,9 @@ import java.time.format.DateTimeFormatter | |||
import java.time.temporal.Temporal | |||
import java.util.UUID | |||
import scala.annotation.implicitNotFound | |||
import scala.collection.immutable.BitSet |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can clean that import
case object InvalidInputArgument extends NoStackTrace { | ||
override def getMessage: String = "invalid input argument" | ||
|
||
private[caliban] def apply(expected: String, actual: String): ExecutionError = |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
expected
and actual
are a bit misleading because it sounds like they should be equal like in tests.
How about argType
and argValue
?
We could potentially accept Any
for ArgValue
to avoid doing toString
multiple times too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah I think that's better 👍
As discussed on Discord: https://discord.com/channels/629491597070827530/633200096393166868/1204421751942152193
One downside is that for some ArgBuilders (UUID, temporal) we were already defining inner throwables. I don't think that's very necessary (given that for temporal ABs we add the message to the ExecutionError message. WDUT?