-
Notifications
You must be signed in to change notification settings - Fork 130
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
stop excluding null values when filtering using notEqual #114
stop excluding null values when filtering using notEqual #114
Conversation
Equality with null values behaves differently. If you apply
I'm voting not to approve this PR. @ITDancer13 @Biarity |
Hm interesting - my first thought was 'Yes, I agree with @hasanmanzak' but I'm not sure about it anymore. If we would do the same on a list via Linq it would return @Biarity what do you think? If we would accept it, it should go to v3. |
I still vote for no. Because, if we approve this, it'll be a breaking change for LINQ-to-SQL implementations. LINQ-to-Object implementations however, will benefit I'm sure, but still, we need to keep the maximum compatibility. I'm OK with a switch based configuration, though. |
I prefer merging this in, because the default behvaiour we have now is not 100% expected behviour for the average developer and will probably result in a few bugs, plus we don't have explicit null comparison operators. Let's have nulls included, but adding a configuration option to allow turning this feature off for devs who know what they're doing. |
@AnasZakarneh You accidentally merged master into your branch and not the releases/2.5.0 |
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.
ISSUE_TEMPLAE/bug_report.md
ISSUE_TEMPLATE/config.yml
README.md
should not be part of this PR
a09bcbf
to
aee43a3
Compare
aee43a3
to
aba0f55
Compare
@ITDancer13 , sorry it was by mistake. |
@AnasZakarneh Thanks for merging release/2.5.0 As you can see there were some discussion with @Biarity and @hasanmanzak about this change. Can you please add the suggested configuration option to enable / disable this feature. Default to should like it was before to get a non-breaking change for release 2.5.0 |
@ITDancer13 Thanks for reviewing the PR. |
* Setup release 2.5.0 with automated build and pre-releases * #80 added support for escaping pipe control characters (#113) * #80 added support for escaping comma and pipe control characters * Update SieveModel.cs Fix build. Accidentally broken by resolving conflicts. * Migrate UnitTests to xUnit Co-authored-by: Clayton Andersen <[email protected]> Co-authored-by: ITDancer13 <[email protected]> Co-authored-by: ITDancer139 <[email protected]> * SieveProcessor.Options made protected property (#134) Mapper assignment in constructor is moved to a null-coalescing member pair (a field and a property) "IncludeScopes" switch is removed from appSettings.{env}.json files * Revert to _mapper assignment in constructor. (#140) * reverting fix (#142) * Revert to _mapper assignment in constructor. * reverting fix * pass filter values as parameters (#112) make GetClosureOverConstant really work * Make ApplyFiltering, ApplySorting and ApplyPagination protected virtual #139 (#144) * stop excluding null values when filtering using notEqual (#114) * stop excluding null values when filtering using notEqual * add IgnoreNullsOnNotEqual config field, to enable/disable the new feature Co-authored-by: AnasZakarneh <[email protected]> Co-authored-by: Clayton Andersen <[email protected]> Co-authored-by: Clayton Andersen <[email protected]> Co-authored-by: ITDancer139 <[email protected]> Co-authored-by: Hasan Manzak <[email protected]> Co-authored-by: alicak <[email protected]> Co-authored-by: AnasZakarneh <[email protected]> Co-authored-by: AnasZakarneh <[email protected]>
ApplyFiltering is modified to be able to return null values when the operator is not equal.
if we have [{categoryId=null}, {categoryId=1}, {categoryId=2}]
and we aplly "categoryId!=2" the result should be [{categoryId=null}, {categoryId=1}] not [{categoryId=1}]