8000 V2 null checks by yln · Pull Request #114 · morelinq/MoreLINQ · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

V2 null checks #114

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 2 commits into from
Sep 21, 2015
Merged

V2 null checks #114

merged 2 commits into from
Sep 21, 2015

Conversation

yln
Copy link
Contributor
@yln yln commented Sep 19, 2015

Fixes #110

@yln
Copy link
Contributor Author
yln commented Sep 21, 2015

The following is just my (subjective) opinion. I am fine either way.

  1. I agree with "an overload with additional parameters should behave as if those parameters are optional".
  2. As I see it, the default behavior is throwing a InvalidOperationException and technically the default value is not null, but delegate { return null; }, but that is just an implementation detail. (The fact that the default behavior is implemented via a delegate that returns null bothers me a little too.)
  3. Why provide two ways to do the same thing?
    source.Assert(predicate) and source.Assert(predicate, null)
    For this method errorSelector will almost never be a computed value (which would force us to put an if/else in our code).
  4. I prefer the "everything-non-null" approach (unless explicitly stated). I can't tell from the method signature that null is okay here. It's not even stated in the docs. Can the predicate be null too? (I agree that this particular method wouldn't make much sense then, but in general ...)

I agree that there is the exception for IEqualityComparer<> and IComparer<>, because we need to be consistent with the .NET framework (a design decision made 10? years ago).
I disagree with the other exceptions (maybe 2.x is a good point to introduce minor breaking changes to clean them up?) and especially with adding new exceptions.

@atifaziz
Copy link
Member

Ah, I'm sorry, I completely misread that you were talking about Assert and not AssertCount because none of your points was lining up to what I was seeing in the code. Anyway, since we agree on the first point, let's move on to the remaining ones…

The fact that the default behavior is implemented via a delegate that returns null bothers me a little too

This was to avoid duplication in the code by way of having the policy at one location.

Why provide two ways to do the same thing?

You could see it that way but the actual intention was that faced with a faulty errorSelector implementation (one returning a null for exception), Assert did the right thing. We can now argue whether hiding, making or patching a faulty implementation is a good thing or not. I do see that AssertCount and Assert are inconsistent here as the former will fault with NullReferenceException when it gets to throwing the exception and the delegate has returned a null.

I can't tell from the method signature that null is okay here. It's not even stated in the docs.

We've talked about this. Optionality is not covered by the type system but in this case, it's about covering a faulty implementation that didn't seem worth bloating docs for. Why? Well, the errorSelector enables three things: (1) keep raising InvalidOperationException but change the message, (2) change the raise exception type or (3) both. If the delegate implementation intended to change the message, the exception type or both and yet by error returned null then the defaulted implementation of Assert would yield a different result. Either it would be caught by unit tests or observed behaviour at runtime. That all said…

I prefer the "everything-non-null" approach

I can go with this for the return value of errorSelector but the entire delegate should remain optional with respect to the other overload(s).

atifaziz added a commit that referenced this pull request Sep 21, 2015
@atifaziz
Merge pull request #114 from yln/v2-null-checks
17d3993 
Tests look good:

```
NUnit-Console version 2.6.2.12296
Copyright (C) 2002-2012 Charlie Poole.
Copyright (C) 2002-2004 James W. Newkirk, Michael C. Two, Alexei A. Vorontsov.
Copyright (C) 2000-2002 Philip Craig.
All Rights Reserved.

Runtime Environment - 
   OS Version: Microsoft Windows NT 6.2.9200.0
  CLR Version: 2.0.50727.8662 ( Net 3.5 )

ProcessModel: Default    DomainUsage: Single
Execution Runtime: net-3.5
..................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................
Tests run: 722, Errors: 0, Failures: 0, Inconclusive: 0, Time: 3.5310994 seconds
  Not run: 0, Invalid: 0, Ignored: 0, Skipped: 0
```

@yln Thank for all your help with this and keen observations. I'm merging your latest contributions and we can continue the thread on `Assert` in another issue if you think it's important to reconsider for 2.0.
@atifaziz atifaziz merged commit 17d3993 into morelinq:v2-null-checks Sep 21, 2015
@atifaziz
Copy link
Member

Tests look good:

NUnit-Console version 2.6.2.12296
Copyright (C) 2002-2012 Charlie Poole.
Copyright (C) 2002-2004 James W. Newkirk, Michael C. Two, Alexei A. Vorontsov.
Copyright (C) 2000-2002 Philip Craig.
All Rights Reserved.

Runtime Environment - 
   OS Version: Microsoft Windows NT 6.2.9200.0
  CLR Version: 2.0.50727.8662 ( Net 3.5 )

ProcessModel: Default    DomainUsage: Single
Execution Runtime: net-3.5
..................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................
Tests run: 722, Errors: 0, Failures: 0, Inconclusive: 0, Time: 3.5310994 seconds
  Not run: 0, Invalid: 0, Ignored: 0, Skipped: 0

@yln Thank for all your help with this and keen observations. I'm merging your latest contributions and we can continue the thread on Assert in another issue if you think it's important to reconsider for 2.0.

P.S. I pasted this response in the wrong box, and sadly, it became part of the commit message detail. 🙈

atifaziz added a commit that referenced this pull request Sep 22, 2015
@atifaziz
Merge pull request #114 from yln/v2-null-checks
ba2274f
@atifaziz
Copy link
Member

⚠️ I've done the unthinkable and re-pushed (read modified history) the merge as ba2274f with the commit message fixed. There are only two known forks at this point in time so I've exceptionally gone ahead and made the change rather than live with the silly commit error forever as part of the repo history.

🙊

@yln yln deleted the v2-null-checks branch September 30, 2015 00:09
@atifaziz atifaziz mentioned this pull request Oct 1, 2015
Closed
@atifaziz atifaziz added the merged label Mar 3, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
merged
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@yln @atifaziz
0