-
Notifications
You must be signed in to change notification settings - Fork 7
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
Report type when erroring on unsupported-type #15
Conversation
Codecov Report
@@ Coverage Diff @@
## master #15 +/- ##
==========================================
- Coverage 100% 98.19% -1.81%
==========================================
Files 13 13
Lines 811 831 +20
Branches 0 4 +4
==========================================
+ Hits 811 816 +5
- Misses 0 11 +11
- Partials 0 4 +4
Continue to review full report at Codecov.
|
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.
Seems like a useful addition to the error case - could you also cover this with a test?
@@ -922,7 +922,8 @@ | |||
(error/*handler* | |||
::unsupported-type | |||
(str "No known encoding for object: " (pr-str x)) | |||
{:value x}))) | |||
{:value x | |||
:type (type x)}))) |
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.
Two thoughts here - first, class
is generally more appropriate than type
, since the latter will return you (:type (meta x))
which is (obviously) user-modifiable. Given that the default dispatch function is class
this would also match the behavior better.
Second thought builds on the first one, which is that this case occurs when write-handled
falls through because there's no handler for the dispatched type. It may be more useful to make this data include the dispatch
function's return value, since that's what's actually used for the handler behavior.
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.
Going to merge this so you have a commit, will make the change I suggested directly.
When logging the value, it's not always clear the exact type that's causing failure (
ArrayList
vsvec
for example). Adding the type makes this error message a little more informative.