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

V2 null consistency #116

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
Closed
wants to merge 6 commits into from
Closed

V2 null consistency #116

wants to merge 6 commits into from

Conversation

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

Remove "nullable" exceptions (other than IEqualityComparer and IComparer) for consistency. Refine implementations and update docs.

  • Assert(errorSelector): now consistent with AssertCount
  • ToDataTable(expressions): params array, so no reason why this should ever be null; there wasn't even a test to cover the null case
  • Trace(format): format string should never be null (never computed, fixed per call-site); also simplify implementation (string.Format(format, args) handles null just fine)
  • ToDelimitedString(delimiter): Make it more explicit what happens; also update docs.

@atifaziz atifaziz closed this Oct 1, 2015
@yln
Copy link
Contributor Author
yln commented Oct 1, 2015

I guess this was auto-closed because the base-branch went away. Any comments on the changes?

@atifaziz
Copy link
Member
atifaziz commented Oct 1, 2015

That was my bad. I was too eager to close the branch v2-null-checks branch after merging with master, not realising you still had v2-null-consistency open over it.

@atifaziz atifaziz reopened this Oct 1, 2015
@atifaziz
Copy link
Member
atifaziz commented Oct 1, 2015

I'm not too happy with d75ed05 and we've talked about this at some length in #114. Overloads with mere additional arguments should act as if they are optional and provide identical behaviour to those overloads where the arguments are omitted. Throwing an exception means the result cannot be computed because it would be undefined and that's not true.

The goal here is not to simplify the test code.

@atifaziz atifaziz mentioned this pull request Oct 1, 2015
Closed
@yln
Copy link
Contributor Author
yln commented Oct 2, 2015

source.ToDelimitedString() is not equal to source.ToDelimitedString(null), it is equal to source.ToDelimitedString(CultureInfo.CurrentCulture.TextInfo.ListSeparator).

I can't imagine any scenario in which I would want to call source.ToDelimitedString(null). delimiter will most likely not be a computed value. Even if it is a computed value, it is much better if the user defines what the fallback for null is.

Which code is more explicit/less surprising?

var delimiter = ComputeDelimiter();
var x = source.ToDelimitedString(delimiter);
var delimiter = ComputeDelimiter() ?? ",";
var x = source.ToDelimitedString(delimiter);

As a casual reader I have no chance of knowing that delimiter will be replaced with CultureInfo.CurrentCulture.TextInfo.ListSeparator (which even varies by system) in variant 1.
(Yes, of course, I would also argue for disallowing ComputeDelimiter() to return null.)

The rest of the changes are okay?

@fsateler
Copy link
Member
fsateler commented Oct 7, 2015

I'm missing a changes file where it is documented this breaking behavior change (is this done only on the nuspec file at release time?).

FWIW, I agree with @yln that ToDelimitedString(null) makes no sense. I don't think "default value for a parameter" needs to be equal to "default value for the parameter type".

@yln
Copy link
Contributor Author
yln commented Oct 8, 2015

I'm missing a changes file where it is documented this breaking behavior change (is this done only on the nuspec file at release time?).

I can provide that if you guys tell me which file and format it should have.

@fsateler
Copy link
Member

@yln I don't think such a file exists currently.

@atifaziz
Copy link
Member
atifaziz commented Nov 26, 2015 •
edited
Loading

source.ToDelimitedString() is not equal to source.ToDelimitedString(null), it is equal to source.ToDelimitedString(CultureInfo.CurrentCulture.TextInfo.ListSeparator).

source.ToDelimitedString() is equal to source.ToDelimitedString(null) as both exhibit the same runtime behaviour as if you wrote source.ToDelimitedString(CultureInfo.CurrentCulture.TextInfo.ListSeparator) explicitly.

As a casual reader I have no chance of knowing that delimiter will be replaced with CultureInfo.CurrentCulture.TextInfo.ListSeparator (which even varies by system) in variant 1.

Casual readers have no chance of knowing what DateTime.Now.ToString(), DateTime.Now.ToString(null) or string.Format(DateTime.Now) do if they haven't the slightest clue or ever read a line or two of the docs; or define casual reading. Yes, they give you string representations of the date & time but the exact format and output is defined by the environment. In general, such code is found in presentation & UI layers. In other cases where you need certainty, you supply arguments explicitly.

The ToDelimitedString overloads accepting a delimiter allow null in much the same way that many text-formatting methods in BCL permit a null for format specification, IFormatProvider and so forth. If you want to force a delimiter then we should remove the simple overload that takes no arguments because maintaining consistency in the optionality of the parameters across the overloads is more about contractual correctness than implementation tactics.

That all said, is ToDelimitedString() at all useful? Not really. I actually added it just because it could be done & TextInfo.ListSeparator was available as a reasonable choice. I personally only ever use it when doing casual experimentation or demos in, say, LINQPad. Even then I usually I prefer to specify the delimiter in case an experiment is so successful that I end up taking it to production. I don't want to have to unnecessarily revisit the code & add delimiters back. When I added the zero-arity overload, I maintained the optionality of the argument for consistency. So again, I think the real question to ask is whether to keep or drop the ToDelimitedString nullary overload, not who in their right mind would compute the delimiter with a possibility of null. I would personally opt to drop it but I don't find it's doing any harm at the moment either, especially not worth delaying the 2.0 release & this PR has thrown a 🔧 at that (which is a shame because given your help & contributions, I wanted to reciprocate by getting 2.0 out the door).

On a further aside on optionality, do bear in mind that there is also the spirit & idea in LINQ (especially with standard query operators) to be able to project or translate quoted queries into foreign systems. The simpler & non-explicit overloads do help there where the exact runtime semantics are understood to be defined by the target environment. I'm certainly not suggesting that ToDelimitedString was designed with that scenario in mind or that I'd like to maintain it as-is to exclude potential opportunities.

@yln
Copy link
Contributor Author
yln commented Nov 30, 2015

Okay, you convinced me. I don't have a strong opinion any more. ;)

@atifaziz atifaziz mentioned this pull request Jan 4, 2017
Closed
@atifaziz atifaziz closed this Jan 4, 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
wontfix
Projects
None yet
Milestone
No milestone
Development

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