-
Notifications
You must be signed in to change notification settings - Fork 934
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
Replace SafetyEnumerable<T> with OfType<T> where applicable #2368
Replace SafetyEnumerable<T> with OfType<T> where applicable #2368
Conversation
@@ -91,11 +91,15 @@ public void AddColumns(IEnumerable<Column> columnIterator) | |||
{ | |||
foreach (Column col in columnIterator) | |||
{ | |||
if (!col.IsFormula) |
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.
IsFormula
is not virtual and always false for Column
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.
There is only one place where it matters -
HQLQueryPlan
(as user can expect null from results).
This usage looks to me as a bug "hider", silently ignoring invalid result rows (not having the expected type). Shouldn't it be a Cast<T>
here instead?
I wonder if the change is a good move, because in case some bug introduces a null
value in those collections, the previous class would let it flow through and eventually cause trouble to consumer code, while with OfType
it will be silently filtered out. But maybe that is more a trouble of missing null checks where we add things to these collections.
Yeah.. Indeed.. Only with special null handling for structs. Something like:
I would say yes to this. And regarding columns handling - null values didn't flow nicely to consumer code before. They stopped pretty quickly in
|
After another thought I made this change in this PR. |
Obsolete
SafetyEnumerable<T>
by replacing withOfType<T>
extension.Exception
HQLQueryPlan
whereCast
is applied (as more appropriate behavior in this context)