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
Assign error code when concatenation fails #113
Assign error code when concatenation fails #113
Conversation
Feel free to give me some feedback, I am not a Kotlin person at all |
1170a1a
to
c07acdc
Compare
@@ -77,8 +77,11 @@ fun ExprValue.numberValue(): Number = | |||
fun ExprValue.timestampValue(): Timestamp = | |||
scalar.timestampValue() ?: errNoContext("Expected timestamp: $ionValue", internal = false) | |||
|
|||
fun ExprValue.stringValue(): String = | |||
scalar.stringValue() ?: errNoContext("Expected text: $ionValue", internal = false) | |||
fun ExprValue.stringValue(): String = scalar.stringValue() ?: |
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.
This is a general extension function that is called in multiple use cases that require to obtain a String
from an ExprValue
, one such case is when we need to evaluate a string append ||
operator.
Making this extension function report an error that always states it is due to a string append operation might lead to miscommunication.
I think the logic needs to go here where the code deals with string append. Each argument to ||
needs to be checked and if it is not a String
value then report the more specific error message that ||
did not receive a value of the appropriate type (String
)
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.
Makes sense - I'll attempt to fix it before next week.
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.
@therapon Can anyone take this over? A lot going on in my life, I would love to help you out but I cannot do that. Should I just trash this PR?
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.
Definitely don't delete it. One of us will take it over when there's time. Thanks.
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.
No problem, I'm glad I could contribute something meaningful.
#131 continuing this work |
#131 was pushed Thanks @pawelduda for your contribution :) |
Issue #27, if available:
Description of changes:
Solves issue #27
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.