-
Notifications
You must be signed in to change notification settings - Fork 1.9k
Issue 4047, improve error message give expected type #5189
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -4,6 +4,7 @@ | |
|
||
using System; | ||
using System.Collections.Generic; | ||
using System.Linq; | ||
using System.Threading; | ||
using System.Threading.Tasks; | ||
using System.Threading.Tasks.Dataflow; | ||
|
@@ -731,9 +732,12 @@ public override ValueGetter<TValue> GetGetter<TValue>(DataViewSchema.Column colu | |
{ | ||
Ch.CheckParam(column.Index < _colToActivesIndex.Length, nameof(column)); | ||
Ch.CheckParam(_colToActivesIndex[column.Index] >= 0, nameof(column), "requested column not active"); | ||
ValueGetter<TValue> getter = _getters[_colToActivesIndex[column.Index]] as ValueGetter<TValue>; | ||
|
||
var originGetter = _getters[_colToActivesIndex[column.Index]]; | ||
ValueGetter<TValue> getter = originGetter as ValueGetter<TValue>; | ||
if (getter == null) | ||
throw Ch.Except("Invalid TValue: '{0}'", typeof(TValue)); | ||
throw Ch.Except($"Invalid TValue: '{typeof(TValue)}', " + | ||
$"expected type: '{originGetter.GetType().GetGenericArguments().First()}'."); | ||
Comment on lines
+735
to
+740
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. (pinned to code to allow for threaded conversation) In general, I think that the approach you used to fix this issue is fine, and it's actually a very good addition to make our exception messages more clear. The only potential problem I see with it is that maybe this will end up throwing another exception: for example, the I haven't checked the other places were you've made changes, but some of them do have an original I'm not sure if I'm explaining myself. In any case, I also don't know if we should worry about this problem... that other exception would only be thrown in some very fringe case (which I'm not sure if it's actually possible to reach that case), and in any way we were already throwing an exception. So maybe this is unimportant. #Resolved There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I understand your concern, please check out the implementation of this _getters, mainly method CreateGetterDelegate in same class and you will find below implementation, so it is guaranteed you will get ValueGetter for this Delegate but the real problem is you are using wrong TValue type.
In reply to: 433925403 [](ancestors = 433925403) |
||
return getter; | ||
} | ||
} | ||
|
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.
In here we expect a VBuffer, and that's why you're getting .GetGenericArguments() twice, which is fine to get a descriptive exception.
But what happens if _getter is, let's say, of type
ValueGetter<int>
whereas we expect one of typeValueGetter<VBuffer<float>>
? since the getter we're getting isn't aVBuffer<>
then another exception will be thrown when calling the second.First()
because the second.GetGenericArguments()
would return an empty array. This other exception would be more confusing than the original one, so please add the necessary logic to handle this case correctly in the places where we expect a VBuffer. 😄 #ResolvedThere 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.
check out the definition of _getter: private readonly ValueGetter<VBuffer> _getter;
In reply to: 433952241 [](ancestors = 433952241)