Skip to content
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

Merged
merged 2 commits into from
May 12, 2020

Conversation

bahusoid
Copy link
Member

@bahusoid bahusoid commented Apr 27, 2020

Obsolete SafetyEnumerable<T> by replacing with OfType<T> extension.
Exception HQLQueryPlan where Cast is applied (as more appropriate behavior in this context)

@@ -91,11 +91,15 @@ public void AddColumns(IEnumerable<Column> columnIterator)
{
foreach (Column col in columnIterator)
{
if (!col.IsFormula)
Copy link
Member Author

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

Copy link
Member

@fredericDelaporte fredericDelaporte left a 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.

@bahusoid
Copy link
Member Author

bahusoid commented May 10, 2020

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 here instead?

Yeah.. Indeed.. Only with special null handling for structs. Something like:
Select( obj => obj == null ? (default(T)) : (T)obj);
But I think such fix better to do in separate PR as possible breaking change.

But maybe that is more a trouble of missing null checks where we add things to these collections.

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 AddColumns:

@bahusoid
Copy link
Member Author

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 here instead?

After another thought I made this change in this PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants